r109631 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109630‎ | r109631 | r109632 >
Date:18:38, 20 January 2012
Author:gregchiasson
Status:resolved (Comments)
Tags:aft 
Comment:
AFT5 - sorting doesnt work right (feedbackpage) but the designers want the latest HTML, so here it is.
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
——@@ -57,9 +57,11 @@
5858 'articlefeedbackv5-special-filter-helpful' => 'Helpful ($1)',
5959 'articlefeedbackv5-special-filter-visible' => 'Visible ($1)',
6060 'articlefeedbackv5-special-filter-invisible' => 'Hidden ($1)',
61 - 'articlefeedbackv5-special-sort-newest' => 'Newest first',
62 - 'articlefeedbackv5-special-sort-oldest' => 'Oldest first',
63 - 'articlefeedbackv5-special-sort-helpful' => 'Most Helpful',
 61+ 'articlefeedbackv5-special-sort-asc' => '^',
 62+ 'articlefeedbackv5-special-sort-desc' => 'v',
 63+ 'articlefeedbackv5-special-sort-age' => 'Date',
 64+ 'articlefeedbackv5-special-sort-helpfulness' => 'Helpful',
 65+ 'articlefeedbackv5-special-sort-rating' => 'Rating',
6466 'articlefeedbackv5-special-sort-label-before' => 'Sort by:',
6567 'articlefeedbackv5-special-sort-label-after' => '',
6668 'articlefeedbackv5-special-filter-label-before' => 'Show only:',
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
——@@ -50,9 +50,14 @@
5151 /**
5252 * The name of the sorting method used
5353 */
54 - $.articleFeedbackv5special.sort = 'newest';
 54+ $.articleFeedbackv5special.sort = 'age';
5555
5656 /**
 57+ * The dorection of the sorting method used
 58+ */
 59+ $.articleFeedbackv5special.sortDirection = 'desc';
 60+
 61+ /**
5762 * The number of responses to display per data pull
5863 */
5964 $.articleFeedbackv5special.limit = 25;
——@@ -81,9 +86,24 @@
8287 return false;
8388 } );
8489 $( '.articleFeedbackv5-sort-link' ).bind( 'click', function( e ) {
85 - $.articleFeedbackv5special.sort = $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-special-sort-' );
86 - $.articleFeedbackv5special.continue = null;
 90+ id = $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-special-sort-' );
 91+ oldId = $.articleFeedbackv5special.sort;
 92+
 93+ // set direction = desc...
 94+ $.articleFeedbackv5special.sortDirection = 'desc';
 95+ $.articleFeedbackv5special.sort = id;
 96+ $.articleFeedbackv5special.continue = null;
8797 $.articleFeedbackv5special.loadFeedback( true );
 98+
 99+ // unless we're flipping the direction on the current sort.
 100+console.log('id is ' + id + ', old id is ' + oldId);
 101+ if( id == oldId
 102+ && $.articleFeedbackv5special.sortDirection == 'desc') {
 103+ $.articleFeedbackv5special.sortDirection = 'asc';
 104+ }
 105+ // draw arrow
 106+ $.articleFeedbackv5special.drawSortArrow();
 107+
88108 return false;
89109 } );
90110 $( '#articleFeedbackv5-show-more' ).bind( 'click', function( e ) {
——@@ -113,6 +133,18 @@
114134 } );
115135 }
116136
 137+ $.articleFeedbackv5special.drawSortArrow = function() {
 138+ id = $.articleFeedbackv5special.sort;
 139+ dir = $.articleFeedbackv5special.sortDirection;
 140+
 141+ $( '.articleFeedbackv5-sort-arrow' ).hide();
 142+
 143+ $( '#articleFeedbackv5-sort-arrow-' + id ).text(
 144+ mw.msg( 'articlefeedbackv5-special-sort-' + dir )
 145+ );
 146+ $( '#articleFeedbackv5-sort-arrow-' + id ).show();
 147+ }
 148+
117149 // Utility method for stripping long IDs down to the specific bits we care about.
118150 $.articleFeedbackv5special.stripID = function( object, toRemove ) {
119151 return $( object ).attr( 'id' ).replace( toRemove, '' );
——@@ -188,12 +220,13 @@
189221 'type' : 'GET',
190222 'dataType': 'json',
191223 'data' : {
192 - 'afvfpageid' : $.articleFeedbackv5special.page,
193 - 'afvffilter' : $.articleFeedbackv5special.filter,
194 - 'afvffiltervalue' : $.articleFeedbackv5special.filterValue,
195 - 'afvfsort' : $.articleFeedbackv5special.sort,
196 - 'afvflimit' : $.articleFeedbackv5special.limit,
197 - 'afvfcontinue' : $.articleFeedbackv5special.continue,
 224+ 'afvfpageid' : $.articleFeedbackv5special.page,
 225+ 'afvffilter' : $.articleFeedbackv5special.filter,
 226+ 'afvffiltervalue' : $.articleFeedbackv5special.filterValue,
 227+ 'afvfsort' : $.articleFeedbackv5special.sort,
 228+ 'afvfsortdirection' : $.articleFeedbackv5special.sortDirection,
 229+ 'afvflimit' : $.articleFeedbackv5special.limit,
 230+ 'afvfcontinue' : $.articleFeedbackv5special.continue,
198231 'action' : 'query',
199232 'format' : 'json',
200233 'list' : 'articlefeedbackv5-view-feedback',
——@@ -253,6 +286,7 @@
254287
255288 // Initial load
256289 $.articleFeedbackv5special.loadFeedback( true );
 290+ $.articleFeedbackv5special.drawSortArrow();
257291
258292 // }}}
259293
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
——@@ -38,6 +38,7 @@
3939 $params['filter'],
4040 $params['filtervalue'],
4141 $params['sort'],
 42+ $params['sortdirection'],
4243 $params['limit'],
4344 ( $params['continue'] !== 'null' ? $params['continue'] : null )
4445 );
——@@ -75,28 +76,31 @@
7677 }
7778
7879 public function fetchFeedback( $pageId,
79 - $filter = 'visible', $filterValue = null, $order = 'newest', $limit = 25, $continue = null ) {
 80+ $filter = 'visible', $filterValue = null, $sort = 'age', $sortOrder = 'desc', $limit = 25, $continue = null ) {
8081 $dbr = wfGetDB( DB_SLAVE );
8182 $ids = array();
8283 $rows = array();
8384 $rv = array();
8485 $where = $this->getFilterCriteria( $filter, $filterValue );
 86+
 87+ $direction = strtolower( $sortOrder ) == 'asc' ? 'ASC' : 'DESC';
 88+ $continueDirection = ( $direction == 'ASC' ? '>' : '<' );
8589 $order;
 90+ $continue;
8691
87 - # TODO: The SQL needs to handle all sorts of weird cases.
88 - switch( $order ) {
 92+ switch( $sort ) {
8993 case 'helpful':
90 - $order = 'net_helpfulness DESC';
91 - $continueSql = 'net_helpfulness <';
 94+ $order = "net_helpfulness $direction";
 95+ $continueSql = "net_helpfulness $continueDirection";
9296 break;
93 - case 'oldest':
94 - $order = 'af_id ASC';
95 - $continueSql = 'af_id >';
 97+ case 'rating':
 98+ # For now, just fall through. Not specced out.
9699 break;
97 - case 'newest':
 100+ case 'age':
 101+ # Default sort is by age.
98102 default:
99 - $order = 'af_id DESC';
100 - $continueSql = 'af_id <';
 103+ $order = "af_id $direction";
 104+ $continueSql = "af_id $continueDirection";
101105 break;
102106 }
103107
——@@ -184,6 +188,27 @@
185189 return $rv;
186190 }
187191
 192+ private function getContinue( $sort, $sortOrder ) {
 193+ $continue;
 194+ $direction = strtolower( $sortOrder ) == 'asc' ? '<' : '>';
 195+
 196+ switch( $sort ) {
 197+ case 'helpful':
 198+ $continue = 'net_helpfulness <';
 199+ break;
 200+ case 'rating':
 201+ # For now, just fall through. Not specced out.
 202+ break;
 203+ case 'age':
 204+ # Default sort is by age.
 205+ default:
 206+ $continue = 'af_id >';
 207+ break;
 208+ }
 209+
 210+ return $continue;
 211+ }
 212+
188213 private function getFilterCriteria( $filter, $filterValue = null ) {
189214 $where = array();
190215
——@@ -315,10 +340,14 @@
316341 . Html::closeElement( 'div' );
317342
318343 return Html::openElement( 'div', array( 'class' => 'articleFeedbackv5-feedback' ) )
 344+ . Html::openElement( 'div' => array(
 345+ 'class' => 'articleFeedbackv5-comment-wrap'
 346+ )
319347 . $content
 348+ . $footer_links
 349+ . Html::closeElement( 'div' )
320350 . $details
321351 . $tools
322 - . $footer_links
323352 . $rate
324353 . Html::closeElement( 'div' );
325354 }
——@@ -409,34 +438,40 @@
410439 */
411440 public function getAllowedParams() {
412441 return array(
413 - 'pageid' => array(
 442+ 'pageid' => array(
414443 ApiBase::PARAM_REQUIRED => true,
415444 ApiBase::PARAM_ISMULTI => false,
416445 ApiBase::PARAM_TYPE => 'integer'
417446 ),
418 - 'sort' => array(
 447+ 'sort' => array(
419448 ApiBase::PARAM_REQUIRED => false,
420449 ApiBase::PARAM_ISMULTI => false,
421450 ApiBase::PARAM_TYPE => array(
422 - 'oldest', 'newest', 'helpful' )
 451+ 'age', 'helpfulness', 'rating' )
423452 ),
424 - 'filter' => array(
 453+ 'sortdirection' => array(
425454 ApiBase::PARAM_REQUIRED => false,
426455 ApiBase::PARAM_ISMULTI => false,
427456 ApiBase::PARAM_TYPE => array(
 457+ 'desc', 'asc' )
 458+ ),
 459+ 'filter' => array(
 460+ ApiBase::PARAM_REQUIRED => false,
 461+ ApiBase::PARAM_ISMULTI => false,
 462+ ApiBase::PARAM_TYPE => array(
428463 'all', 'invisible', 'visible', 'comment', 'id' )
429464 ),
430 - 'filtervalue' => array(
 465+ 'filtervalue' => array(
431466 ApiBase::PARAM_REQUIRED => false,
432467 ApiBase::PARAM_ISMULTI => false,
433468 ApiBase::PARAM_TYPE => 'string'
434469 ),
435 - 'limit' => array(
 470+ 'limit' => array(
436471 ApiBase::PARAM_REQUIRED => false,
437472 ApiBase::PARAM_ISMULTI => false,
438473 ApiBase::PARAM_TYPE => 'integer'
439474 ),
440 - 'continue' => array(
 475+ 'continue' => array(
441476 ApiBase::PARAM_REQUIRED => false,
442477 ApiBase::PARAM_ISMULTI => false,
443478 ApiBase::PARAM_TYPE => 'string'
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.hooks.php
——@@ -162,7 +162,9 @@
163163 'articlefeedbackv5-delete-saved',
164164 'articlefeedbackv5-helpful-saved',
165165 'articlefeedbackv5-unhelpful-saved',
166 - 'articlefeedbackv5-comment-link'
 166+ 'articlefeedbackv5-comment-link',
 167+ 'articlefeedbackv5-special-sort-asc',
 168+ 'articlefeedbackv5-special-sort-desc'
167169 ),
168170 ),
169171 );

Follow-up revisions

RevisionCommit summaryAuthorDate
r109632AFT5 sigh. This is why you test before you commit. Syntax errors, fixed now. ...gregchiasson18:39, 20 January 2012
r109634r109631: Update ignore messagesraymond18:48, 20 January 2012

Comments

#Comment by Catrope (talk | contribs)   06:09, 31 January 2012 
+			id    = $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-special-sort-' );
+			oldId = $.articleFeedbackv5special.sort;

This makes the id and oldId variables global, use var id = $.foobar(); 

+		id  = $.articleFeedbackv5special.sort;
+		dir = $.articleFeedbackv5special.sortDirection;

Same. 

+console.log('id is ' + id + ', old id is ' + oldId);

Debugging code, removed in r109645 

 		$order;
+		$continue;

What are these lines here for? You don't have to declare variables before using them in PHP.

#Comment by Catrope (talk | contribs)   06:58, 31 January 2012

Missing var fixed in r110359.

Status & tagging log