r110520 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110519‎ | r110520 | r110521 >
Date:21:16, 1 February 2012
Author:gregchiasson
Status:resolved (Comments)
Tags:aft 
Comment:
AFT5: Fix query-continue support for rating/helpfulness sorting. Refs r110412, stil has a lingering issue with ratings sort.
Modified paths:
  • /trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/sql/alter.sql (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedbackv5/sql/alter.sql
——@@ -122,3 +122,6 @@
123123 ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_net_helpfulness integer NOT NULL DEFAULT 0;
124124 CREATE INDEX /*i*/af_net_helpfulness ON /*_*/aft_article_feedback (af_net_helpfulness);
125125 UPDATE aft_article_feedback SET af_net_helpfulness = CONVERT(af_helpful_count, SIGNED) - CONVERT(af_unhelpful_count, SIGNED);
 126+
 127+-- Added 2/1 (greg)
 128+CREATE INDEX /*_*/af_net_helpfulness_af_id ON /*_*/aft_article_feedback (af_id, af_net_helpfulness);
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
——@@ -51,7 +51,8 @@
5252 sort: 'age',
5353 sortDirection: 'desc',
5454 limit: 25,
55 - continue: null
 55+ continue: null,
 56+ continueId: null // Sort of a tie-breaker for continue values.
5657 };
5758
5859 /**
——@@ -110,8 +111,9 @@
111112 */
112113 $.articleFeedbackv5special.setBinds = function() {
113114 $( '#articleFeedbackv5-filter-select' ).bind( 'change', function( e ) {
114 - $.articleFeedbackv5special.listControls.filter = $(this).val();
115 - $.articleFeedbackv5special.listControls.continue = null;
 115+ $.articleFeedbackv5special.listControls.filter = $(this).val();
 116+ $.articleFeedbackv5special.listControls.continue = null;
 117+ $.articleFeedbackv5special.listControls.continueId = null;
116118 $.articleFeedbackv5special.loadFeedback( true );
117119 return false;
118120 } );
——@@ -121,8 +123,9 @@
122124 oldId = $.articleFeedbackv5special.listControls.sort;
123125
124126 // set direction = desc...
125 - $.articleFeedbackv5special.listControls.sort = id;
126 - $.articleFeedbackv5special.listControls.continue = null;
 127+ $.articleFeedbackv5special.listControls.sort = id;
 128+ $.articleFeedbackv5special.listControls.continue = null;
 129+ $.articleFeedbackv5special.listControls.continueId = null;
127130
128131 // unless we're flipping the direction on the current sort.
129132 if( id == oldId && $.articleFeedbackv5special.listControls.sortDirection == 'desc' ) {
——@@ -148,6 +151,7 @@
149152 $.articleFeedbackv5special.listControls.filter = 'id';
150153 $.articleFeedbackv5special.listControls.filterValue = id;
151154 $.articleFeedbackv5special.listControls.continue = null;
 155+ $.articleFeedbackv5special.listControls.continueId = null;
152156 $.articleFeedbackv5special.loadFeedback( true );
153157 } );
154158
——@@ -579,6 +583,7 @@
580584 'afvfsortdirection' : $.articleFeedbackv5special.listControls.sortDirection,
581585 'afvflimit' : $.articleFeedbackv5special.listControls.limit,
582586 'afvfcontinue' : $.articleFeedbackv5special.listControls.continue,
 587+ 'afvfcontinueid' : $.articleFeedbackv5special.listControls.continueId,
583588 'action' : 'query',
584589 'format' : 'json',
585590 'list' : 'articlefeedbackv5-view-feedback',
——@@ -617,7 +622,8 @@
618623 }
619624 } );
620625 $( '#articleFeedbackv5-feedback-count-total' ).text( data['articlefeedbackv5-view-feedback'].count );
621 - $.articleFeedbackv5special.listControls.continue = data['articlefeedbackv5-view-feedback'].continue;
 626+ $.articleFeedbackv5special.listControls.continue = data['articlefeedbackv5-view-feedback'].continue;
 627+ $.articleFeedbackv5special.listControls.continueId = data['articlefeedbackv5-view-feedback'].continueid;
622628 } else {
623629 $( '#articleFeedbackv5-show-feedback' ).text( mw.msg( 'articlefeedbackv5-error-loading-feedback' ) );
624630 }
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
——@@ -14,7 +14,8 @@
1515 * @subpackage Api
1616 */
1717 class ApiViewFeedbackArticleFeedbackv5 extends ApiQueryBase {
18 - private $continue;
 18+ private $continue = null;
 19+ private $continueId = null;
1920
2021 /**
2122 * Constructor
——@@ -40,21 +41,23 @@
4142 $params['sort'],
4243 $params['sortdirection'],
4344 $params['limit'],
44 - ( $params['continue'] !== 'null' ? $params['continue'] : null )
 45+ ( $params['continue'] !== 'null' ? $params['continue'] : null ),
 46+ ( $params['continueid'] !== 'null' ? $params['continueid'] : null )
4547 );
4648 foreach ( $feedback as $record ) {
4749 $html .= $this->renderFeedback( $record );
4850 $length++;
4951 }
5052
51 - $continue = $this->continue;
52 -
5353 $result->addValue( $this->getModuleName(), 'length', $length );
5454 $result->addValue( $this->getModuleName(), 'count', $count );
 55+ if ( $this->continue !== null ) {
 56+ $result->addValue( $this->getModuleName(), 'continue', $this->continue );
 57+ }
 58+ if ( $this->continueId ) {
 59+ $result->addValue( $this->getModuleName(), 'continueid', $this->continueId );
 60+ }
5561 $result->addValue( $this->getModuleName(), 'feedback', $html );
56 - if ( $continue ) {
57 - $result->addValue( $this->getModuleName(), 'continue', $continue );
58 - }
5962 }
6063
6164 public function fetchFeedbackCount( $pageId ) {
——@@ -74,7 +77,7 @@
7578
7679 public function fetchFeedback( $pageId, $filter = 'visible',
7780 $filterValue = null, $sort = 'age', $sortOrder = 'desc',
78 - $limit = 25, $continue = null ) {
 81+ $limit = 25, $continue = null, $continueId ) {
7982 $dbr = wfGetDB( DB_SLAVE );
8083 $ids = array();
8184 $rows = array();
——@@ -103,28 +106,37 @@
104107 // Build ORDER BY clause.
105108 switch( $sort ) {
106109 case 'helpful':
107 - $sortField = 'af_net_helpfulness';
 110+ $sortField = 'af_net_helpfulness';
 111+ $order = "af_net_helpfulness $direction, af_id $direction";
 112+ $continueSql = "(af_net_helpfulness $continueDirection ".intVal( $continue )
 113+ ." OR (af_net_helpfulness = ".intVal( $continue )
 114+ ." AND af_id $continueDirection ".intval( $continueId ).") )";
108115 break;
109116 case 'rating':
110 - $sortField = 'rating';
 117+ # TODO: null ratings don't seem to show up at all. Need to sort that one out.
 118+ $sortField = 'rating';
 119+ $order = "rating $direction, af_id $direction";
 120+ $continueSql = "(rating.aa_response_boolean $continueDirection ".intVal( $continue )
 121+ ." OR (rating.aa_response_boolean = ".intVal( $continue )
 122+ ." AND af_id $continueDirection ".intval( $continueId ).") )";
111123 break;
112124 case 'age':
113125 # Default field, fall through
114126 default:
115 - $sortField = 'af_id';
 127+ $sortField = 'af_id';
 128+ $order = "af_id $direction";
 129+ $continueSql = "af_id < ".intVal( $continue );
116130 break;
117131 }
118 - $order = "$sortField $direction";
119 - $continueSql = "$sortField $continueDirection";
120132
121133 // Build WHERE clause.
122134 // Filter applied , if any:
123135 $where = $this->getFilterCriteria( $filter, $filterValue );
124136 // PageID:
125137 $where['af_page_id'] = $pageId;
126 - // Continue value, if any:
 138+ // Continue SQL, if any:
127139 if ( $continue !== null ) {
128 - $where[] = $continueSql.' '.intVal( $continue );
 140+ $where[] = $continueSql;
129141 }
130142 // Only show bucket 1 (per Fabrice on 1/25)
131143 $where['af_bucket_id'] = 1;
——@@ -163,10 +175,13 @@
164176 )
165177 )
166178 );
 179+
167180 foreach ( $id_query as $id ) {
168181 $ids[] = $id->af_id;
169 - $this->continue = $id->$sortField;
 182+ $this->continue = $id->$sortField;
 183+ $this->continueId = $id->af_id;
170184 }
 185+
171186
172187 if ( !count( $ids ) ) {
173188 return array();
——@@ -683,6 +698,11 @@
684699 ApiBase::PARAM_ISMULTI => false,
685700 ApiBase::PARAM_TYPE => 'integer'
686701 ),
 702+ 'continueid' => array(
 703+ ApiBase::PARAM_REQUIRED => false,
 704+ ApiBase::PARAM_ISMULTI => false,
 705+ ApiBase::PARAM_TYPE => 'string'
 706+ ),
687707 'continue' => array(
688708 ApiBase::PARAM_REQUIRED => false,
689709 ApiBase::PARAM_ISMULTI => false,
——@@ -703,7 +723,8 @@
704724 'filter' => 'What filtering to apply to list',
705725 'filtervalue' => 'Optional param to pass to filter',
706726 'limit' => 'Number of records to show',
707 - 'continue' => 'Offset from which to continue',
 727+ 'continue' => 'sort value at which to continue',
 728+ 'continueid' => 'Last af_id with that sort value that was shown (IE, where to continue if the page break has the same sort value)',
708729 );
709730 }
710731

Follow-up revisions

RevisionCommit summaryAuthorDate
r113062bug 34090 - follow up to r110520 1. change index 2. default of null for conti...emsmith18:54, 5 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r110412Remainder of the changes from r109565 - adds net_helpfulness as an actual col...gregchiasson17:54, 31 January 2012

Comments

#Comment by Catrope (talk | contribs)   00:38, 28 February 2012 
+CREATE INDEX /*_*/af_net_helpfulness_af_id ON /*_*/aft_article_feedback (af_id, af_net_helpfulness);

This index is the wrong way around, because you're doing: 

+ $order = "af_net_helpfulness $direction, af_id $direction";

- $limit = 25, $continue = null ) {
+ $limit = 25, $continue = null, $continueId ) {

Adding a non-optional parameter after an optional parameter is bad form, please make this one optional as well (default to null is probably fine). 

+ $order = "rating $direction, af_id $direction";
+ $continueSql = "(rating.aa_response_boolean $continueDirection ".intVal( $continue )

It took me a while to figure out that you have a table called 'rating' and a field alias called 'rating', and that the latter is an alias for rating.aa_response_boolean . That's really confusing, please don't do that.

The query that orders by rating looks like it's not going to sort efficiently. I couldn't tell you offhand how it could be written more efficiently, because of the excessive normalization going on. The page ID and the rating value are in different tables, so I don't think you're gonna be able to pull that off without cross-table indexes (which MySQL doesn't have). Also, why are you selecting and sorting on aa_response_boolean here? Isn't the rating an integer type rather than a boolean type?

OK otherwise.

#Comment by Elizabeth M Smith (talk | contribs)   18:54, 5 March 2012

the "rating" for bucket 1 is actually "yes" or "no" for "did you find what you were looking for" since this doesn't use a numerical rating - so a boolean value

#Comment by Catrope (talk | contribs)   18:56, 5 March 2012

Aaaah, OK, and I guess this only deals with bucket 1 stuff then. So that makes sense.

#Comment by Elizabeth M Smith (talk | contribs)   20:43, 5 March 2012

Followed up in r113062

Status & tagging log