r109441 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109440‎ | r109441 | r109442 >
Date:22:00, 18 January 2012
Author:gregchiasson
Status:resolved (Comments)
Tags:aft 
Comment:
AFT5 feedback page: enable permalinks and 'edits since post' functionality. Fix a couple of issues with the translation.
Modified paths:
  • /trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.hooks.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php
——@@ -60,8 +60,8 @@
6161 'articlefeedbackv5-special-filter-label-before' => 'Show only:',
6262 'articlefeedbackv5-special-filter-label-after' => '',
6363 'articlefeedbackv5-special-showing' => '$1 feedback posts on this page', // FIXME: Needs plural support on $1.
64 - 'articleFeedbackv5-link' => 'Permalink',
65 - 'articleFeedbackv5-updates-since' => '$1 Edits since post',
 64+ 'articlefeedbackv5-comment-link' => 'Permalink',
 65+ 'articlefeedbackv5-updates-since' => '{{PLURAL:$1|1 edit|$1 edits}} since post',
6666 'articlefeedbackv5-special-more' => 'More',
6767 'articlefeedbackv5-special-pagetitle' => 'Feedback: $1',
6868
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
——@@ -43,6 +43,11 @@
4444 $.articleFeedbackv5special.filter = 'visible';
4545
4646 /**
 47+ * Some fitlers have values they need tobe passed (eg, permalinks)
 48+ */
 49+ $.articleFeedbackv5special.filterValue = undefined;
 50+
 51+ /**
4752 * The name of the sorting method used
4853 */
4954 $.articleFeedbackv5special.sort = 'newest';
——@@ -50,7 +55,7 @@
5156 /**
5257 * The number of responses to display per data pull
5358 */
54 - $.articleFeedbackv5special.limit = 5;
 59+ $.articleFeedbackv5special.limit = 25;
5560
5661 /**
5762 * The index at which to start the pull
——@@ -225,11 +230,12 @@
226231 'type' : 'GET',
227232 'dataType': 'json',
228233 'data' : {
229 - 'afvfpageid' : $.articleFeedbackv5special.page,
230 - 'afvffilter' : $.articleFeedbackv5special.filter,
231 - 'afvfsort' : $.articleFeedbackv5special.sort,
232 - 'afvflimit' : $.articleFeedbackv5special.limit,
233 - 'afvfcontinue': $.articleFeedbackv5special.continue,
 234+ 'afvfpageid' : $.articleFeedbackv5special.page,
 235+ 'afvffilter' : $.articleFeedbackv5special.filter,
 236+ 'afvffiltervalue' : $.articleFeedbackv5special.filterValue,
 237+ 'afvfsort' : $.articleFeedbackv5special.sort,
 238+ 'afvflimit' : $.articleFeedbackv5special.limit,
 239+ 'afvfcontinue' : $.articleFeedbackv5special.continue,
234240 'action' : 'query',
235241 'format' : 'json',
236242 'list' : 'articlefeedbackv5-view-feedback',
——@@ -278,6 +284,16 @@
279285 $.articleFeedbackv5special.apiUrl = mw.util.wikiScript('api');
280286 $.articleFeedbackv5special.page = mw.config.get( 'afPageId' );
281287 $.articleFeedbackv5special.setBinds();
 288+
 289+ // Process anything we found in the URL hash
 290+ // Permalinks.
 291+ var id = window.location.hash.match(/id=(\d+)/)
 292+ if( id ) {
 293+ $.articleFeedbackv5special.filter = 'id';
 294+ $.articleFeedbackv5special.filterValue = id[1];
 295+ }
 296+
 297+ // Initial load
282298 $.articleFeedbackv5special.loadFeedback( true );
283299
284300 // }}}
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
——@@ -31,10 +31,11 @@
3232 $pageId = $params['pageid'];
3333 $html = '';
3434 $length = 0;
35 - $count = $this->fetchFeedbackCount( $params['pageid'], $params['filter'] );
 35+ $count = $this->fetchFeedbackCount( $params['pageid'], $params['filter'], $params['filtervalue'] );
3636 $feedback = $this->fetchFeedback(
3737 $params['pageid'],
3838 $params['filter'],
 39+ $params['filtervalue'],
3940 $params['sort'],
4041 $params['limit'],
4142 ( $params['continue'] !== 'null' ? $params['continue'] : null )
——@@ -57,7 +58,7 @@
5859 }
5960 }
6061
61 - public function fetchFeedbackCount( $pageId, $filter ) {
 62+ public function fetchFeedbackCount( $pageId, $filter, $filterValue ) {
6263 $dbr = wfGetDB( DB_SLAVE );
6364 $count = $dbr->selectField(
6465 array( 'aft_article_filter_count' ),
——@@ -73,12 +74,12 @@
7475 }
7576
7677 public function fetchFeedback( $pageId,
77 - $filter = 'visible', $order = 'newest', $limit = 25, $continue = null ) {
 78+ $filter = 'visible', $filterValue = null, $order = 'newest', $limit = 25, $continue = null ) {
7879 $dbr = wfGetDB( DB_SLAVE );
7980 $ids = array();
8081 $rows = array();
8182 $rv = array();
82 - $where = $this->getFilterCriteria( $filter );
 83+ $where = $this->getFilterCriteria( $filter, $filterValue );
8384 $order;
8485
8586 # TODO: The SQL needs to handle all sorts of weird cases.
——@@ -118,6 +119,7 @@
119120 'ORDER BY' => $order
120121 )
121122 );
 123+
122124 foreach ( $id_query as $id ) {
123125 $ids[] = $id->af_id;
124126 }
——@@ -136,7 +138,7 @@
137139 'aa_response_rating', 'aa_response_option_id',
138140 'afi_data_type', 'af_created', 'user_name',
139141 'af_user_ip', 'af_hide_count', 'af_abuse_count',
140 - 'af_helpful_count', 'af_delete_count'
 142+ 'af_helpful_count', 'af_delete_count', '(SELECT COUNT(*) FROM revision WHERE rev_id > af_revision_id AND rev_page = '.( integer ) $pageId.') AS age'
141143 ),
142144 array( 'af_id' => $ids ),
143145 __METHOD__,
——@@ -170,7 +172,7 @@
171173 return $rv;
172174 }
173175
174 - private function getFilterCriteria( $filter ) {
 176+ private function getFilterCriteria( $filter, $filterValue = null ) {
175177 $where = array();
176178 switch( $filter ) {
177179 case 'all':
——@@ -182,6 +184,9 @@
183185 case 'comment':
184186 $where = array( 'aa_response_text IS NOT NULL');
185187 break;
 188+ case 'id':
 189+ $where = array( 'af_id' => $filterValue );
 190+ break;
186191 case 'visible':
187192 default:
188193 $where = array( 'af_hide_count' => 0 );
——@@ -224,12 +229,12 @@
225230 'class' => 'articleFeedbackv5-comment-details-permalink'
226231 ) )
227232 .Html::element( 'a', array(
228 - 'href' => '#'
 233+ 'href' => "#id=$id"
229234 ), wfMessage( 'articlefeedbackv5-comment-link' ) )
230235 . Html::closeElement( 'div' )
231236 . Html::element( 'div', array(
232237 'class' => 'articleFeedbackv5-comment-details-updates'
233 - ), wfMessage( 'articleFeedbackv5-updates-since', 0 ) )
 238+ ), wfMessage( 'articlefeedbackv5-updates-since', $record[0]->age ) )
234239 . Html::closeElement( 'div' );
235240 ;
236241
——@@ -272,9 +277,7 @@
273278 . Html::closeElement( 'ul' )
274279 . Html::closeElement( 'div' );
275280
276 - # TODO: permalinks
277281 return Html::openElement( 'div', array( 'class' => 'articleFeedbackv5-feedback' ) )
278 - . $header_links
279282 . $content
280283 . $details
281284 . $tools
——@@ -369,29 +372,34 @@
370373 */
371374 public function getAllowedParams() {
372375 return array(
373 - 'pageid' => array(
 376+ 'pageid' => array(
374377 ApiBase::PARAM_REQUIRED => true,
375378 ApiBase::PARAM_ISMULTI => false,
376379 ApiBase::PARAM_TYPE => 'integer'
377380 ),
378 - 'sort' => array(
 381+ 'sort' => array(
379382 ApiBase::PARAM_REQUIRED => false,
380383 ApiBase::PARAM_ISMULTI => false,
381384 ApiBase::PARAM_TYPE => array(
382385 'oldest', 'newest' )
383386 ),
384 - 'filter' => array(
 387+ 'filter' => array(
385388 ApiBase::PARAM_REQUIRED => false,
386389 ApiBase::PARAM_ISMULTI => false,
387390 ApiBase::PARAM_TYPE => array(
388 - 'all', 'invisible', 'visible', 'comment' )
 391+ 'all', 'invisible', 'visible', 'comment', 'id' )
389392 ),
390 - 'limit' => array(
 393+ 'filtervalue' => array(
391394 ApiBase::PARAM_REQUIRED => false,
392395 ApiBase::PARAM_ISMULTI => false,
 396+ ApiBase::PARAM_TYPE => 'string'
 397+ ),
 398+ 'limit' => array(
 399+ ApiBase::PARAM_REQUIRED => false,
 400+ ApiBase::PARAM_ISMULTI => false,
393401 ApiBase::PARAM_TYPE => 'integer'
394402 ),
395 - 'continue' => array(
 403+ 'continue' => array(
396404 ApiBase::PARAM_REQUIRED => false,
397405 ApiBase::PARAM_ISMULTI => false,
398406 ApiBase::PARAM_TYPE => 'string'
——@@ -406,11 +414,12 @@
407415 */
408416 public function getParamDescription() {
409417 return array(
410 - 'pageid' => 'Page ID to get feedback ratings for',
411 - 'sort' => 'Key to sort records by',
412 - 'filter' => 'What filtering to apply to list',
413 - 'limit' => 'Number of records to show',
414 - 'continue' => 'Offset from which to continue',
 418+ 'pageid' => 'Page ID to get feedback ratings for',
 419+ 'sort' => 'Key to sort records by',
 420+ 'filter' => 'What filtering to apply to list',
 421+ 'filtervalue' => 'Optional param to pass to filter',
 422+ 'limit' => 'Number of records to show',
 423+ 'continue' => 'Offset from which to continue',
415424 );
416425 }
417426
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.hooks.php
——@@ -161,7 +161,6 @@
162162 'articlefeedbackv5-delete-saved',
163163 'articlefeedbackv5-helpful-saved',
164164 'articlefeedbackv5-comment-link'
165 -
166165 ),
167166 ),
168167 );

Follow-up revisions

RevisionCommit summaryAuthorDate
r110425AFT5: Remove the evil COUNT(*) subquery, and replace the link it was used it ...gregchiasson20:38, 31 January 2012

Comments

#Comment by Catrope (talk | contribs)   05:59, 31 January 2012 
+				'af_helpful_count', 'af_delete_count', '(SELECT COUNT(*) FROM revision WHERE rev_id > af_revision_id AND rev_page = '.( integer ) $pageId.') AS age'

Subqueries with COUNT(*) make me sad. Is this really a required feature? If so, I'd like to try to convince Fabrice to drop it.

Status & tagging log