r71057 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r71056‎ | r71057 | r71058 >
Date:23:43, 13 August 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
implement banner pagination for edit campaign interface
Modified paths:
  • /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/TemplatePager.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/TemplatePager.php
——@@ -23,81 +23,168 @@
2424 $this->viewPage = SpecialPage::getTitleFor( 'NoticeTemplate', 'view' );
2525 }
2626
 27+ /**
 28+ * Pull all banners from the database
 29+ */
2730 function getQueryInfo() {
2831 return array(
2932 'tables' => 'cn_templates',
3033 'fields' => array( 'tmp_name', 'tmp_id' ),
3134 );
3235 }
33 -
 36+
 37+ /**
 38+ * Sort the banner list by tmp_id
 39+ */
3440 function getIndexField() {
3541 return 'tmp_id';
3642 }
3743
 44+ /**
 45+ * Generate the content of each table row (1 row = 1 banner)
 46+ */
3847 function formatRow( $row ) {
39 - $htmlOut = Xml::openElement( 'tr' );
 48+
 49+ $templatesAssigned = array();
 50+ // If we are calling the pager from the campaign editing interface...
 51+ if ( $this->special->mName == 'CentralNotice' ) {
 52+ // Find banners already assigned to the campaign
 53+ $notice = $this->mRequest->getVal( 'notice' );
 54+ $templatesAssigned = $this->selectTemplatesAssigned( $notice );
 55+ }
4056
41 - if ( $this->editable ) {
42 - // Remove box
 57+ // If banner is not already assigned...
 58+ if ( !in_array ( $row->tmp_name, $templatesAssigned ) ) {
 59+
 60+ // Begin banner row
 61+ $htmlOut = Xml::openElement( 'tr' );
 62+
 63+ if ( $this->editable ) {
 64+ // If we are calling the pager from the manage banners interface...
 65+ if ( $this->special->mName == 'NoticeTemplate' ) {
 66+ // Remove box
 67+ $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
 68+ Xml::check( 'removeTemplates[]', false,
 69+ array(
 70+ 'value' => $row->tmp_name,
 71+ 'onchange' => $this->onRemoveChange
 72+ )
 73+ )
 74+ );
 75+ // Else, if we are calling the pager from the campaign editing interface...
 76+ } elseif ( $this->special->mName == 'CentralNotice' ) {
 77+ // Add box
 78+ $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
 79+ Xml::check( 'addTemplates[]', '', array ( 'value' => $row->tmp_name ) )
 80+ );
 81+ // Weight select
 82+ $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
 83+ Xml::listDropDown( "weight[$row->tmp_name]",
 84+ CentralNotice::dropDownList( wfMsg( 'centralnotice-weight' ), range ( 0, 100, 5 ) ) ,
 85+ '',
 86+ '25',
 87+ '',
 88+ '' )
 89+ );
 90+ }
 91+ }
 92+
 93+ // Link and Preview
 94+ $viewPage = SpecialPage::getTitleFor( 'NoticeTemplate', 'view' );
 95+ $render = new SpecialNoticeText();
 96+ $render->project = 'wikipedia';
 97+ $render->language = $this->mRequest->getVal( 'wpUserLanguage' );
4398 $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
44 - Xml::check( 'removeTemplates[]', false,
45 - array(
46 - 'value' => $row->tmp_name,
47 - 'onchange' => $this->onRemoveChange
48 - )
 99+ $this->getSkin()->makeLinkObj( $this->viewPage,
 100+ htmlspecialchars( $row->tmp_name ),
 101+ 'template=' . urlencode( $row->tmp_name ) ) .
 102+ Xml::fieldset( wfMsg( 'centralnotice-preview' ),
 103+ $render->getHtmlNotice( $row->tmp_name ),
 104+ array( 'class' => 'cn-bannerpreview')
49105 )
50106 );
 107+
 108+ // End banner row
 109+ $htmlOut .= Xml::closeElement( 'tr' );
51110 }
52 -
53 - // Link and Preview
54 - $render = new SpecialNoticeText();
55 - $render->project = 'wikipedia';
56 - $render->language = $this->mRequest->getVal( 'wpUserLanguage' );
57 - $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
58 - $this->getSkin()->makeLinkObj( $this->viewPage,
59 - htmlspecialchars( $row->tmp_name ),
60 - 'template=' . urlencode( $row->tmp_name ) ) .
61 - Xml::fieldset( wfMsg( 'centralnotice-preview' ),
62 - $render->getHtmlNotice( $row->tmp_name ),
63 - array( 'class' => 'cn-bannerpreview')
64 - )
65 - );
66 -
67 - $htmlOut .= Xml::closeElement( 'tr' );
 111+
68112 return $htmlOut;
69113 }
70114
 115+ /**
 116+ * Specify table headers
 117+ */
71118 function getStartBody() {
72119 $htmlOut = '';
73 -
74 - $htmlOut .= Xml::openElement( 'table',
75 - array(
76 - 'cellpadding' => 9,
77 - 'width' => '100%'
78 - )
79 - );
 120+ $htmlOut .= Xml::openElement( 'table', array( 'cellpadding' => 9 ) );
 121+ $htmlOut .= Xml::openElement( 'tr' );
80122 if ( $this->editable ) {
81 - $htmlOut .= Xml::element( 'th', array( 'align' => 'left', 'width' => '5%' ),
82 - wfMsg ( 'centralnotice-remove' )
83 - );
 123+ if ( $this->special->mName == 'CentralNotice' ) {
 124+ $htmlOut .= Xml::element( 'th', array( 'align' => 'left', 'width' => '5%' ),
 125+ wfMsg ( "centralnotice-add" )
 126+ );
 127+ $htmlOut .= Xml::element( 'th', array( 'align' => 'left', 'width' => '5%' ),
 128+ wfMsg ( "centralnotice-weight" )
 129+ );
 130+ } elseif ( $this->special->mName == 'NoticeTemplate' ) {
 131+ $htmlOut .= Xml::element( 'th', array( 'align' => 'left', 'width' => '5%' ),
 132+ wfMsg ( 'centralnotice-remove' )
 133+ );
 134+ }
84135 }
85136 $htmlOut .= Xml::element( 'th', array( 'align' => 'left' ),
86 - wfMsg ( 'centralnotice-banner-name' )
 137+ wfMsg ( 'centralnotice-templates' )
87138 );
 139+ $htmlOut .= Xml::closeElement( 'tr' );
88140 return $htmlOut;
89141 }
90142
 143+ /**
 144+ * Close table and add Submit button if we're on the Manage banners page
 145+ */
91146 function getEndBody() {
92147 global $wgUser;
93148 $htmlOut = '';
94149 $htmlOut .= Xml::closeElement( 'table' );
95 - if ( $this->editable ) {
96 - $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
97 - $htmlOut .= Xml::tags( 'div',
98 - array( 'class' => 'cn-buttons' ),
99 - Xml::submitButton( wfMsg( 'centralnotice-modify' ) )
100 - );
 150+ if ( $this->special->mName == 'NoticeTemplate' ) {
 151+ if ( $this->editable ) {
 152+ $htmlOut .= Xml::hidden( 'authtoken', $wgUser->editToken() );
 153+ $htmlOut .= Xml::tags( 'div',
 154+ array( 'class' => 'cn-buttons' ),
 155+ Xml::submitButton( wfMsg( 'centralnotice-modify' ) )
 156+ );
 157+ }
101158 }
102159 return $htmlOut;
103160 }
 161+
 162+ /**
 163+ * Build a list of all the banners assigned to a campaign
 164+ * @return An array of banner names
 165+ */
 166+ function selectTemplatesAssigned ( $notice ) {
 167+ $dbr = wfGetDB( DB_SLAVE );
 168+ $res = $dbr->select(
 169+ array(
 170+ 'cn_notices',
 171+ 'cn_assignments',
 172+ 'cn_templates'
 173+ ),
 174+ array(
 175+ 'cn_templates.tmp_name',
 176+ ),
 177+ array(
 178+ 'cn_notices.not_name' => $notice,
 179+ 'cn_notices.not_id = cn_assignments.not_id',
 180+ 'cn_assignments.tmp_id = cn_templates.tmp_id'
 181+ ),
 182+ __METHOD__,
 183+ array( 'ORDER BY' => 'cn_notices.not_id' )
 184+ );
 185+ $templateNames = array();
 186+ foreach ( $res as $row ) {
 187+ array_push( $templateNames, $row->tmp_name ) ;
 188+ }
 189+ return $templateNames;
 190+ }
104191 }
Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
——@@ -924,67 +924,19 @@
925925 * Create form for adding banners to a campaign
926926 */
927927 function addTemplatesForm( $notice ) {
928 - global $wgUser;
929 - $sk = $wgUser->getSkin();
 928+ $pager = new TemplatePager( $this );
930929 $dbr = wfGetDB( DB_SLAVE );
931930 $res = $dbr->select( 'cn_templates', 'tmp_name', '', '', array( 'ORDER BY' => 'tmp_id' ) );
932931
933932 if ( $dbr->numRows( $res ) > 0 ) {
934933 // Build HTML
935934 $htmlOut = Xml::fieldset( wfMsg( "centralnotice-available-templates" ) );
936 - $htmlOut .= Xml::openElement( 'table', array( 'cellpadding' => 9 ) );
937 -
938 - $htmlOut .= Xml::element( 'th', array( 'align' => 'left', 'width' => '5%' ),
939 - wfMsg ( "centralnotice-add" ) );
940 - $htmlOut .= Xml::element( 'th', array( 'align' => 'left', 'width' => '5%' ),
941 - wfMsg ( "centralnotice-weight" ) );
942 - $htmlOut .= Xml::element( 'th', array( 'align' => 'left', 'width' => '70%' ),
943 - wfMsg ( "centralnotice-templates" ) );
944 -
945 - // Find dups
946 - $templatesAssigned = $this->selectTemplatesAssigned( $notice );
947 -
948 - // Build rows
949 - while ( $row = $dbr->fetchObject( $res ) ) {
950 - if ( !in_array ( $row->tmp_name, $templatesAssigned ) ) {
951 - $htmlOut .= Xml::openElement( 'tr' );
952 -
953 - // Add
954 - $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
955 - Xml::check( 'addTemplates[]', '', array ( 'value' => $row->tmp_name ) )
956 - );
957 -
958 - // Weight
959 - $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
960 - Xml::listDropDown( "weight[$row->tmp_name]",
961 - $this->dropDownList( wfMsg( 'centralnotice-weight' ), range ( 0, 100, 5 ) ) ,
962 - '',
963 - '25',
964 - '',
965 - '' )
966 - );
967 -
968 - // Render preview
969 - $viewPage = $this->getTitleFor( 'NoticeTemplate', 'view' );
970 - $render = new SpecialNoticeText();
971 - $render->project = 'wikipedia';
972 - global $wgRequest;
973 - $render->language = $wgRequest->getVal( 'wpUserLanguage' );
974 - $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ),
975 - $sk->makeLinkObj( $viewPage,
976 - htmlspecialchars( $row->tmp_name ),
977 - 'template=' . urlencode( $row->tmp_name ) ) .
978 - Xml::fieldset( wfMsg( 'centralnotice-preview' ),
979 - $render->getHtmlNotice( $row->tmp_name ),
980 - array( 'class' => 'cn-bannerpreview')
981 - )
982 - );
983 -
984 - $htmlOut .= Xml::closeElement( 'tr' );
985 - }
986 - }
987 -
988 - $htmlOut .= Xml::closeElement( 'table' );
 935+
 936+ // Show paginated list of banners
 937+ $htmlOut .= Xml::tags( 'div', array( 'class' => 'cn-pager' ), $pager->getNavigationBar() );
 938+ $htmlOut .= $pager->getBody();
 939+ $htmlOut .= Xml::tags( 'div', array( 'class' => 'cn-pager' ), $pager->getNavigationBar() );
 940+
989941 $htmlOut .= Xml::closeElement( 'fieldset' );
990942 } else {
991943 // Nothing found
——@@ -994,36 +946,6 @@
995947 }
996948
997949 /**
998 - * Build a list of all the banners assigned to a campaign
999 - * @return An array of template names
1000 - */
1001 - function selectTemplatesAssigned ( $notice ) {
1002 - $dbr = wfGetDB( DB_SLAVE );
1003 - $res = $dbr->select(
1004 - array(
1005 - 'cn_notices',
1006 - 'cn_assignments',
1007 - 'cn_templates'
1008 - ),
1009 - array(
1010 - 'cn_templates.tmp_name',
1011 - ),
1012 - array(
1013 - 'cn_notices.not_name' => $notice,
1014 - 'cn_notices.not_id = cn_assignments.not_id',
1015 - 'cn_assignments.tmp_id = cn_templates.tmp_id'
1016 - ),
1017 - __METHOD__,
1018 - array( 'ORDER BY' => 'cn_notices.not_id' )
1019 - );
1020 - $templateNames = array();
1021 - foreach ( $res as $row ) {
1022 - array_push( $templateNames, $row->tmp_name ) ;
1023 - }
1024 - return $templateNames;
1025 - }
1026 -
1027 - /**
1028950 * Lookup function for active banners under a given language and project. This function is
1029951 * called by SpecialNoticeText::getJsOutput() in order to build the static Javascript files for
1030952 * each project.

Follow-up revisions

RevisionCommit summaryAuthorDate
r71060better paging solution for edit campaign interface than r71057kaldari03:55, 14 August 2010
r71595cleaning up queries, switching weight indexes to ids (per r71057), switching ...kaldari22:23, 24 August 2010

Comments

#Comment by Catrope (talk | contribs)   01:14, 24 August 2010 
+						Xml::listDropDown( "weight[$row->tmp_name]",

I recommend using a numerical index instead, say $row->tmp_id. Using text indexes is scary because they might contain interesting things such as ] 

+			array( 'ORDER BY' => 'cn_notices.not_id' )
[...]
+			array_push( $templateNames, $row->tmp_name ) ;

Instead of grabbing the result and inverting it on the client, you should use ORDER BY ... DESC or at least add a comment explaining what you're doing.

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

The array_push code was deleted in r71060.

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

weight Ids fixed in r71595.

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

Why did you not just subclass the pager? It would have saved a lot of 

if ( $this->special->mName == 'CentralNotice' ) {

The code would have been a lot neater as a result.

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

I see you did this in r71580.

Status & tagging log