r85483 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r85482‎ | r85483 | r85484 >
Date:22:59, 5 April 2011
Author:tparscal
Status:resolved (Comments)
Tags:
Comment:
Split expertise query into it's own method, since it's logged per revision not per rating row. Fixed issue where user text and anon token were not being used in the where clause for user ratings, which manifest as crazy results when more than one anon user (user id 0) rates something.
Modified paths:
  • /trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
——@@ -49,9 +49,15 @@
5050 );
5151 }
5252
53 - // User ratings
 53+ // User-specific data
5454 $ratings[$params['pageid']]['status'] = 'current';
5555 if ( $params['userrating'] ) {
 56+ // Expertise
 57+ $expertise = $this->getExpertise( $params );
 58+ if ( $expertise !== false ) {
 59+ $ratings[$params['pageid']]['expertise'] = $expertise;
 60+ }
 61+ // User ratings
5662 $userRatings = $this->getUserRatings( $params );
5763 if ( isset( $ratings[$params['pageid']]['ratings'] ) ) {
5864 // Valid ratings already exist
——@@ -60,11 +66,6 @@
6167 // Rating value
6268 $ratings[$params['pageid']]['ratings'][$i]['userrating'] =
6369 $userRatings[$rating['ratingid']]['value'];
64 - // Expertise
65 - if ( !isset( $ratings[$params['pageid']]['expertise'] ) ) {
66 - $ratings[$params['pageid']]['expertise'] =
67 - $userRatings[$rating['ratingid']]['expertise'];
68 - }
6970 // Expiration
7071 if ( $userRatings[$rating['ratingid']]['revision'] < $revisionLimit ) {
7172 $ratings[$params['pageid']]['status'] = 'expired';
——@@ -93,10 +94,6 @@
9495 'count' => 0,
9596 'userrating' => $userRating['value'],
9697 );
97 - // Expertise
98 - if ( !isset( $ratings[$params['pageid']]['expertise'] ) ) {
99 - $ratings[$params['pageid']]['expertise'] = $userRating['expertise'];
100 - }
10198 }
10299 }
103100 }
——@@ -108,11 +105,10 @@
109106
110107 $result->setIndexedTagName_internal( array( 'query', $this->getModuleName() ), 'aa' );
111108 }
112 -
113 - protected function getUserRatings( $params ) {
114 - global $wgUser, $wgArticleFeedbackRatings;
115 -
116 - $pageId = $params['pageid'];
 109+
 110+ protected function getAnonToken( $params ) {
 111+ global $wgUser;
 112+
117113 $token = '';
118114 if ( $wgUser->isAnon() ) {
119115 if ( !isset( $params['anontoken'] ) ) {
——@@ -122,36 +118,49 @@
123119 }
124120 $token = $params['anontoken'];
125121 }
 122+ return $token;
 123+ }
 124+
 125+ protected function getExpertise( $params ) {
 126+ global $wgUser;
 127+
 128+ return $this->getDB()->selectField(
 129+ 'article_feedback_properties',
 130+ 'afp_value_text',
 131+ array(
 132+ 'afp_key' => 'expertise',
 133+ 'afp_user_text' => $wgUser->getName(),
 134+ 'afp_user_anon_token' => $this->getAnonToken( $params ),
 135+ ),
 136+ __METHOD__,
 137+ array( 'ORDER BY', 'afp_revision DESC' )
 138+ );
 139+ }
 140+
 141+ protected function getUserRatings( $params ) {
 142+ global $wgUser, $wgArticleFeedbackRatings;
126143
127144 $res = $this->getDB()->select(
128 - array( 'article_feedback', 'article_feedback_properties', 'article_feedback_ratings' ),
 145+ array( 'article_feedback', 'article_feedback_ratings' ),
129146 array(
130147 'aa_rating_id',
131148 'aar_rating',
132149 'aa_revision',
133150 'aa_rating_value',
134 - 'afp_value_text'
135151 ),
136152 array(
137 - 'aa_page_id' => $pageId,
 153+ 'aa_page_id' => $params['pageid'],
138154 'aa_rating_id' => $wgArticleFeedbackRatings,
139155 'aa_user_id' => $wgUser->getId(),
 156+ 'aa_user_text' => $wgUser->getName(),
 157+ 'aa_user_anon_token' => $this->getAnonToken( $params ),
140158 ),
141159 __METHOD__,
142160 array(
143 - 'ORDER BY' => 'aa_revision DESC',
144 - 'LIMIT' => count( $wgArticleFeedbackRatings )
 161+ 'LIMIT' => count( $wgArticleFeedbackRatings ),
 162+ 'ORDER BY' => array( 'aa_rating_id', 'aa_revision DESC' ),
145163 ),
146164 array(
147 - 'article_feedback_properties' => array(
148 - 'LEFT JOIN', array(
149 - 'afp_revision=aa_revision',
150 - 'afp_user_text=aa_user_text',
151 - 'afp_user_anon_token=aa_user_anon_token',
152 - 'aa_user_anon_token' => $token,
153 - 'afp_key' => 'expertise',
154 - )
155 - ),
156165 'article_feedback_ratings' => array( 'LEFT JOIN', array( 'aar_id=aa_rating_id' ) )
157166 )
158167 );
——@@ -165,7 +174,6 @@
166175 if ( $revId === $row->aa_revision ) {
167176 $ratings[$row->aa_rating_id] = array(
168177 'value' => $row->aa_rating_value,
169 - 'expertise' => $row->afp_value_text,
170178 'revision' => $row->aa_revision,
171179 'text' => $row->aar_rating,
172180 );

Follow-up revisions

RevisionCommit summaryAuthorDate
r85912Removed unneeded ORDER BY, which would degrade performance. See r85483tparscal20:02, 12 April 2011
r85914Removed dependence on sorting to get properties from the most current revisio...tparscal20:26, 12 April 2011

Comments

#Comment by Catrope (talk | contribs)   19:40, 12 April 2011 
+			array(
+				'afp_key' => 'expertise',
+				'afp_user_text' => $wgUser->getName(),
+				'afp_user_anon_token' => $this->getAnonToken( $params ),
+			),
+			__METHOD__,
+			array( 'ORDER BY', 'afp_revision DESC' )

This query is bad. The index is on (revision, user_text, anon_token, key) so you can't to an order by on revision and have a constant WHERE on the other three fields and expect the query to be indexed; the index order is wrong for that. Grabbing the revid from the first row returned was better. 

+				'ORDER BY' => array( 'aa_rating_id', 'aa_revision DESC' ),

These changes to the query make it unindexed. The order by aa_rating_id is unnecessary and looks wrong: it's sorting by rating id before revision ID, and I see no reason to order by rating ID at all. Also, mixed-direction ORDER BYs are not indexed. The WHERE on user_text is needed but is also unindexed, we need to fix the index for that: (aa_user_id, aa_page_id, aa_revision) needs to become (aa_user_id, aa_user_token, aa_page_id, aa_revision) or something like that.

Status & tagging log