r73266 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r73265‎ | r73266 | r73267 >
Date:01:04, 18 September 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
more preliminary code for banner allocation viewer (not localizable yet)
Modified paths:
  • /trunk/extensions/CentralNotice/SpecialBannerAllocation.php (modified) (history)
  • /trunk/extensions/CentralNotice/centralnotice.css (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/centralnotice.css
——@@ -31,6 +31,12 @@
3232 #preferences table tr.cn-active-campaign {
3333 background-color: #ddffdd;
3434 }
 35+#preferences table#envpicker {
 36+ margin-bottom: 1em;
 37+}
 38+#preferences table#envpicker td {
 39+ white-space:nowrap;
 40+}
3541 #preferences div.cn-buttons {
3642 padding:1em;
3743 }
Index: trunk/extensions/CentralNotice/SpecialBannerAllocation.php
——@@ -20,7 +20,7 @@
2121 * Handle different types of page requests
2222 */
2323 function execute( $sub ) {
24 - global $wgOut, $wgUser, $wgRequest, $wgScriptPath, $wgNoticeProjects, $wgContLanguageCode;
 24+ global $wgOut, $wgUser, $wgRequest, $wgScriptPath, $wgNoticeProjects, $wgLanguageCode;
2525
2626 // Begin output
2727 $this->setHeaders();
——@@ -52,13 +52,13 @@
5353 $htmlOut .= Xml::element( 'h2', null, 'View banner allocation' );
5454 $htmlOut .= Xml::tags( 'p', null, 'Choose the environment you would like to view banner allocation for:' );
5555
56 - $htmlOut .= Xml::openElement( 'table', array ( 'cellpadding' => 9 ) );
 56+ $htmlOut .= Xml::openElement( 'table', array ( 'id' => 'envpicker', 'cellpadding' => 7 ) );
5757 $htmlOut .= Xml::openElement( 'tr' );
58 - $htmlOut .= Xml::tags( 'td', array( 'style' => 'width: 150px;' ), wfMsgHtml( 'centralnotice-project-name' ) );
 58+ $htmlOut .= Xml::tags( 'td', array( 'style' => 'width: 20%;' ), wfMsgHtml( 'centralnotice-project-name' ) );
5959 $htmlOut .= Xml::openElement( 'td' );
6060 $htmlOut .= Xml::openElement( 'select', array( 'name' => 'project' ) );
6161 foreach ( $wgNoticeProjects as $value ) {
62 - $htmlOut .= Xml::option( $value, $value, false );
 62+ $htmlOut .= Xml::option( $value, $value, $value == 'wikipedia' );
6363 }
6464 $htmlOut .= Xml::closeElement( 'select' );
6565 $htmlOut .= Xml::closeElement( 'td' );
——@@ -68,8 +68,8 @@
6969 $htmlOut .= Xml::openElement( 'td' );
7070 // Make sure the site language is in the list; a custom language code might not have a defined name...
7171 $languages = Language::getLanguageNames( true );
72 - if( !array_key_exists( $wgContLanguageCode, $languages ) ) {
73 - $languages[$wgContLanguageCode] = $wgContLanguageCode;
 72+ if( !array_key_exists( $wgLanguageCode, $languages ) ) {
 73+ $languages[$wgLanguageCode] = $wgLanguageCode;
7474 }
7575 ksort( $languages );
7676 $htmlOut .= Xml::openElement( 'select', array( 'name' => 'language' ) );
——@@ -77,7 +77,7 @@
7878 $htmlOut .= Xml::option(
7979 wfMsg( 'centralnotice-language-listing', $code, $name ),
8080 $code,
81 - false
 81+ $code == 'en'
8282 );
8383 }
8484 $htmlOut .= Xml::closeElement( 'select' );
——@@ -92,7 +92,7 @@
9393 $htmlOut .= Xml::option(
9494 $name,
9595 $code,
96 - false
 96+ $code == 'US'
9797 );
9898 }
9999 $htmlOut .= Xml::closeElement( 'select' );
——@@ -113,10 +113,7 @@
114114
115115 // Handle form submissions
116116 if ( $wgRequest->wasPosted() ) {
117 -
118 - // Show list of banners by default
119117 $this->showList();
120 -
121118 }
122119
123120 // End Banners tab content
——@@ -127,14 +124,50 @@
128125 * Show a list of banners with allocation. Newer banners are shown first.
129126 */
130127 function showList() {
 128+ global $wgRequest, $wgOut, $wgUser, $wgRequest;
 129+
 130+ $sk = $wgUser->getSkin();
 131+ $viewPage = $this->getTitleFor( 'NoticeTemplate', 'view' );
 132+
131133 // Begin building HTML
132134 $htmlOut = '';
133135
134136 // Begin Allocation list fieldset
135137 $htmlOut .= Xml::openElement( 'fieldset', array( 'class' => 'prefsection' ) );
136138
137 - $htmlOut .= "List goes here";
 139+ $htmlOut .= Xml::tags( 'p', null, 'Banner allocation for '.$wgRequest->getVal( 'language' ).'.'.$wgRequest->getVal( 'project' ).' in '.$wgRequest->getVal( 'country' ).':' );
138140
 141+ $bannerLister = new SpecialBannerListLoader();
 142+ $bannerLister->project = $wgRequest->getVal( 'project' );
 143+ $bannerLister->language = $wgRequest->getVal( 'language' );
 144+ $bannerLister->location = $wgRequest->getVal( 'country' );
 145+ $bannerList = $bannerLister->getJsonList();
 146+ $banners = json_decode( $bannerList, true );
 147+ $totalWeight = 0;
 148+ foreach ( $banners as $banner ) {
 149+ $totalWeight += $banner['weight'];
 150+ }
 151+ if ( $banners ) {
 152+ $htmlOut .= Xml::openElement( 'table', array ( 'cellpadding' => 9, 'class' => 'wikitable sortable' ) );
 153+ $htmlOut .= Xml::openElement( 'tr' );
 154+ $htmlOut .= Xml::element( 'th', array( 'width' => '40%' ), 'Percentage' );
 155+ $htmlOut .= Xml::element( 'th', array( 'width' => '60%' ), wfMsg ( "centralnotice-banner" ) );
 156+ $htmlOut .= Xml::closeElement( 'tr' );
 157+ foreach ( $banners as $banner ) {
 158+ $htmlOut .= Xml::openElement( 'tr' );
 159+ $htmlOut .= Xml::openElement( 'td' );
 160+ $htmlOut .= ( $banner['weight'] / $totalWeight ) * 100 . "%";
 161+ $htmlOut .= Xml::closeElement( 'td' );
 162+ $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
 163+ $sk->makeLinkObj( $viewPage, htmlspecialchars( $banner['name'] ), 'template=' . urlencode( $banner['name'] ) )
 164+ );
 165+ $htmlOut .= Xml::closeElement( 'tr' );
 166+ }
 167+ $htmlOut .= Xml::closeElement( 'table' );
 168+ } else {
 169+ $htmlOut .= Xml::tags( 'p', null, 'No banners allocated.' );
 170+ }
 171+
139172 // End Allocation list fieldset
140173 $htmlOut .= Xml::closeElement( 'fieldset' );
141174

Follow-up revisions

RevisionCommit summaryAuthorDate
r73823using strict comparison per comment at r73266kaldari19:50, 27 September 2010

Comments

#Comment by Nikerabbit (talk | contribs)   07:01, 18 September 2010 
+ $code == 'en'

Please prefer === where possible or make otherwise clear what the intention is. 

+ $sk->makeLinkObj( $viewPage, htmlspecialchars( $banner['name'] ), 'template=' . urlencode( $banner['name'] ) )

makeLinkObj is deprecated, use link() instead. 

+ $banners = json_decode( $bannerList, true );

Don't use json_decode directly, use FormatJson::decode. 

+ $htmlOut .= Xml::tags( 'p', null, 'Banner allocation for '.$wgRequest->getVal( 'language' ).'.'.$wgRequest->getVal( 'project' ).' in '.$wgRequest->getVal( 'country' ).':' );

You are outputting user input unescaped, that's bad!


#Comment by Kaldari (talk | contribs)   19:40, 27 September 2010

The last two are fixed in r73404.

#Comment by Kaldari (talk | contribs)   19:50, 27 September 2010

comparison operators fixed in r73823.

#Comment by Siebrand (talk | contribs)   14:05, 18 September 2010
  • $htmlOut .= Xml::tags( 'p', null, 'Banner allocation for '.$wgRequest->getVal( 'language' ).'.'.$wgRequest->getVal( 'project' ).' in '.$wgRequest->getVal( 'country' ).':' );</code: hard coded UI text
  • $htmlOut .= Xml::element( 'th', array( 'width' => '40%' ), 'Percentage' );: hard coded UI text
  • $htmlOut .= Xml::tags( 'p', null, 'No banners allocated.' );: hard coded UI text

#Comment by Kaldari (talk | contribs)   20:18, 20 September 2010

fixed in r73404.

Status & tagging log