r110959 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r110958‎ | r110959 | r110960 >
Date:20:21, 8 February 2012
Author:catrope
Status:ok
Tags:
Comment:
Revert most of r96722. The massive schema change was way overkill given that AFT is being rewritten (as AFTv5). Will reapply some of the saner parts later
Modified paths:
  • /trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php (modified) (history)
  • /trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php (modified) (history)
  • /trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql (modified) (history)
  • /trunk/extensions/ArticleFeedback/sql/RecreatePK.sql (deleted) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedback/sql/RecreatePK.sql
@@ -1,38 +0,0 @@
2 -
3 -
4 -CREATE TABLE /*_*/article_feedback2 (
5 - aa_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
6 - aa_page_id integer unsigned NOT NULL,
7 - aa_user_id integer NOT NULL,
8 - aa_user_text varbinary(255) NOT NULL,
9 - aa_user_anon_token varbinary(32) NOT NULL DEFAULT '',
10 - aa_revision integer unsigned NOT NULL,
11 - aa_timestamp binary(14) NOT NULL DEFAULT '',
12 - aa_rating_id int unsigned NOT NULL,
13 - aa_rating_value int unsigned NOT NULL,
14 - aa_design_bucket int unsigned NOT NULL DEFAULT 0
15 -) /*$wgDBTableOptions*/;
16 -CREATE INDEX /*i*/aa_page_user_token_id ON /*_*/article_feedback2 (aa_page_id, aa_user_text, aa_user_anon_token, aa_id);
17 -CREATE INDEX /*i*/aa_revision ON /*_*/article_feedback2 (aa_revision);
18 -CREATE INDEX /*i*/article_feedback_timestamp ON /*_*/article_feedback2 (aa_timestamp);
19 -
20 -INSERT INTO /*_*/article_feedback2
21 - (aa_page_id, aa_user_id, aa_user_text, aa_user_anon_token, aa_revision, aa_timestamp, aa_rating_id, aa_rating_value, aa_design_bucket)
22 - SELECT aa_page_id, aa_user_id, aa_user_text, aa_user_anon_token, aa_revision, aa_timestamp, aa_rating_id, aa_rating_value, aa_design_bucket
23 - FROM /*_*/article_feedback
24 - ORDER BY aa_revision, aa_user_text, aa_rating_id, aa_user_anon_token;
25 -
26 -DROP TABLE /*_*/article_feedback;
27 -ALTER TABLE /*_*/article_feedback2 RENAME TO /*_*/article_feedback;
Index: trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql
@@ -1,7 +1,5 @@
22 -- Store article feedbacks (user rating per revision)
33 CREATE TABLE IF NOT EXISTS /*_*/article_feedback (
4 - -- Row ID (primary key)
5 - aa_id integer unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
64 -- Foreign key to page.page_id
75 aa_page_id integer unsigned NOT NULL,
86 -- User Id (0 if anon)
@@ -19,10 +17,11 @@
2018 -- Value of the rating (0 is "unrated", else 1-5)
2119 aa_rating_value int unsigned NOT NULL,
2220 -- Which rating widget the user was given. Default of 0 is the "old" design
23 - aa_design_bucket int unsigned NOT NULL DEFAULT 0
 21+ aa_design_bucket int unsigned NOT NULL DEFAULT 0,
 22+ -- 1 vote per user per revision
 23+ PRIMARY KEY (aa_revision, aa_user_text, aa_rating_id, aa_user_anon_token)
2424 ) /*$wgDBTableOptions*/;
25 -CREATE INDEX /*i*/aa_page_user_token_id ON /*_*/article_feedback (aa_page_id, aa_user_text, aa_user_anon_token, aa_id);
26 -CREATE INDEX /*i*/aa_revision ON /*_*/article_feedback (aa_revision);
 25+CREATE INDEX /*i*/aa_user_page_revision ON /*_*/article_feedback (aa_user_id, aa_page_id, aa_revision);
2726 -- Create an index on the article_feedback.aa_timestamp field
2827 CREATE INDEX /*i*/article_feedback_timestamp ON /*_*/article_feedback (aa_timestamp);
2928 CREATE INDEX /*i*/aa_page_id ON /*_*/article_feedback (aa_page_id, aa_timestamp);
Index: trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php
@@ -38,60 +38,57 @@
3939 }
4040
4141 $dbw = wfGetDB( DB_MASTER );
42 -
43 - $pageId = $params['pageid'];
44 - $revisionId = $params['revid'];
45 -
46 - // Build array( rating ID => rating value )
47 - $ratings = array();
48 - foreach ( $wgArticleFeedbackRatingTypes as $ratingID => $unused ) {
49 - $ratings[$ratingID] = intval( $params["r{$ratingID}"] );
50 - }
51 - // Insert the new ratings
52 - $id = $this->insertUserRatings( $pageId, $revisionId, $wgUser, $token, $ratings, $params['bucket'] );
53 -
54 - // Query the previous ratings by this user for this page,
 42+
 43+ // Query the latest ratings by this user for this page,
5544 // possibly for an older revision
56 - // Use aa_id < $id to make sure we get the one before ours even if other ratings were inserted after
57 - // insertUserRatings() but before selectRow(); this prevents race conditions from messing things up.
 45+ // Select from the master to prevent replag-induced bugs
5846 $res = $dbw->select(
5947 'article_feedback',
6048 array( 'aa_rating_id', 'aa_rating_value', 'aa_revision' ),
6149 array(
 50+ 'aa_user_id' => $wgUser->getId(),
6251 'aa_user_text' => $wgUser->getName(),
6352 'aa_page_id' => $params['pageid'],
6453 'aa_rating_id' => array_keys( $wgArticleFeedbackRatingTypes ),
6554 'aa_user_anon_token' => $token,
66 - 'aa_id < ' . intval( $id )
6755 ),
6856 __METHOD__,
6957 array(
70 - 'ORDER BY' => 'aa_id DESC',
 58+ 'ORDER BY' => 'aa_revision DESC',
7159 'LIMIT' => count( $wgArticleFeedbackRatingTypes ),
7260 )
7361 );
 62+
7463 $lastRatings = array();
 64+
7565 foreach ( $res as $row ) {
7666 $lastRatings[$row->aa_rating_id]['value'] = $row->aa_rating_value;
7767 $lastRatings[$row->aa_rating_id]['revision'] = $row->aa_revision;
7868 }
79 -
 69+
 70+ $pageId = $params['pageid'];
 71+ $revisionId = $params['revid'];
 72+
8073 foreach ( $wgArticleFeedbackRatingTypes as $ratingID => $unused ) {
81 - $lastPageRating = false;
82 - $lastRevRating = false;
 74+ $lastRating = false;
 75+ $lastRevision = false;
8376 if ( isset( $lastRatings[$ratingID] ) ) {
84 - $lastPageRating = intval( $lastRatings[$ratingID]['value'] );
85 - if ( intval( $lastRatings[$ratingID]['revision'] ) == $revisionId ) {
86 - $lastRevRating = $lastPageRating;
87 - }
 77+ $lastRating = intval( $lastRatings[$ratingID]['value'] );
 78+ $lastRevision = intval( $lastRatings[$ratingID]['revision'] );
8879 }
89 - $thisRating = intval( $params["r{$ratingID}"] );
 80+
 81+ $thisRating = false;
 82+ if ( isset( $params["r{$ratingID}"] ) ) {
 83+ $thisRating = intval( $params["r{$ratingID}"] );
 84+ }
 85+
 86+ $this->insertRevisionRating( $pageId, $revisionId, $lastRevision, $ratingID, ( $thisRating - $lastRating ),
 87+ $thisRating, $lastRating
 88+ );
9089
91 - // Update counter tables
92 - $this->insertRevisionRating( $pageId, $revisionId, $ratingID, $thisRating - $lastRevRating,
93 - $thisRating, $lastRevRating
94 - );
95 - $this->insertPageRating( $pageId, $ratingID, $thisRating - $lastPageRating, $thisRating, $lastPageRating );
 90+ $this->insertPageRating( $pageId, $ratingID, ( $thisRating - $lastRating ), $thisRating, $lastRating );
 91+
 92+ $this->insertUserRatings( $pageId, $revisionId, $wgUser, $token, $ratingID, $thisRating, $params['bucket'] );
9693 }
9794
9895 $this->insertProperties( $revisionId, $wgUser, $token, $params );
@@ -160,12 +157,13 @@
161158 *
162159 * @param $pageId Integer: Page Id
163160 * @param $revisionId Integer: Revision Id
 161+ * @param $lastRevision Integer: Revision Id of last rating
164162 * @param $ratingId Integer: Rating Id
165163 * @param $updateAddition Integer: Difference between user's last rating (if applicable)
166164 * @param $thisRating Integer|Boolean: Value of the Rating
167165 * @param $lastRating Integer|Boolean: Value of the last Rating
168166 */
169 - private function insertRevisionRating( $pageId, $revisionId, $ratingId, $updateAddition, $thisRating, $lastRating ) {
 167+ private function insertRevisionRating( $pageId, $revisionId, $lastRevision, $ratingId, $updateAddition, $thisRating, $lastRating ) {
170168 $dbw = wfGetDB( DB_MASTER );
171169
172170 // Try to insert a new "totals" row for this page,rev,rating set
@@ -181,21 +179,58 @@
182180 __METHOD__,
183181 array( 'IGNORE' )
184182 );
185 -
186 - // Apply the difference between the previous and new ratings to the current "totals" row
187 - $dbw->update(
188 - 'article_feedback_revisions',
189 - array(
190 - 'afr_total = afr_total + ' . $updateAddition,
191 - 'afr_count = afr_count + ' . $this->getCountChange( $lastRating, $thisRating ),
192 - ),
193 - array(
194 - 'afr_page_id' => $pageId,
195 - 'afr_rating_id' => $ratingId,
196 - 'afr_revision' => $revisionId,
197 - ),
198 - __METHOD__
199 - );
 183+
 184+ // If that succeded in inserting a row, then we are for sure rating a previously unrated
 185+ // revision, and we need to add more information about this rating to the new "totals" row,
 186+ // as well as remove the previous rating values from the previous "totals" row
 187+ if ( $dbw->affectedRows() ) {
 188+ // If there was a previous rating, there should be a "totals" row for it's revision
 189+ if ( $lastRating ) {
 190+ // Deduct the previous rating values from the previous "totals" row
 191+ $dbw->update(
 192+ 'article_feedback_revisions',
 193+ array(
 194+ 'afr_total = afr_total - ' . intval( $lastRating ),
 195+ 'afr_count = afr_count - 1',
 196+ ),
 197+ array(
 198+ 'afr_page_id' => $pageId,
 199+ 'afr_rating_id' => $ratingId,
 200+ 'afr_revision' => $lastRevision
 201+ ),
 202+ __METHOD__
 203+ );
 204+ }
 205+ // Add this rating's values to the new "totals" row
 206+ $dbw->update(
 207+ 'article_feedback_revisions',
 208+ array(
 209+ 'afr_total' => $thisRating,
 210+ 'afr_count' => $thisRating ? 1 : 0,
 211+ ),
 212+ array(
 213+ 'afr_page_id' => $pageId,
 214+ 'afr_rating_id' => $ratingId,
 215+ 'afr_revision' => $revisionId,
 216+ ),
 217+ __METHOD__
 218+ );
 219+ } else {
 220+ // Apply the difference between the previous and new ratings to the current "totals" row
 221+ $dbw->update(
 222+ 'article_feedback_revisions',
 223+ array(
 224+ 'afr_total = afr_total + ' . $updateAddition,
 225+ 'afr_count = afr_count + ' . $this->getCountChange( $lastRating, $thisRating ),
 226+ ),
 227+ array(
 228+ 'afr_page_id' => $pageId,
 229+ 'afr_rating_id' => $ratingId,
 230+ 'afr_revision' => $revisionId,
 231+ ),
 232+ __METHOD__
 233+ );
 234+ }
200235 }
201236 /**
202237 * Calculate the difference between the previous rating and this one
@@ -212,45 +247,55 @@
213248 }
214249
215250 /**
216 - * Inserts the user's ratings for a specific revision
 251+ * Inserts (or Updates, where appropriate) the users ratings for a specific revision
217252 *
218253 * @param $pageId Integer: Page Id
219254 * @param $revisionId Integer: Revision Id
220255 * @param $user User: Current User object
221256 * @param $token Array: Token if necessary
222 - * @param $ratings Array: Keys are rating IDs, values are rating values
 257+ * @param $ratingId Integer: Rating Id
 258+ * @param $ratingValue Integer: Value of the Rating
223259 * @param $bucket Integer: Which rating widget was the user shown
224 - * @return int Return value of $dbw->insertID(). This is the aa_id of the first (MySQL) or last (SQLite) inserted row
225260 */
226 - private function insertUserRatings( $pageId, $revisionId, $user, $token, $ratings, $bucket ) {
 261+ private function insertUserRatings( $pageId, $revisionId, $user, $token, $ratingId, $ratingValue, $bucket ) {
227262 $dbw = wfGetDB( DB_MASTER );
228263
229264 $timestamp = $dbw->timestamp();
230 -
231 - $rows = array();
232 - foreach ( $ratings as $ratingID => $ratingValue ) {
233 - $rows[] = array(
 265+
 266+ $dbw->insert(
 267+ 'article_feedback',
 268+ array(
234269 'aa_page_id' => $pageId,
235270 'aa_user_id' => $user->getId(),
236271 'aa_user_text' => $user->getName(),
237272 'aa_user_anon_token' => $token,
238273 'aa_revision' => $revisionId,
239274 'aa_timestamp' => $timestamp,
240 - 'aa_rating_id' => $ratingID,
 275+ 'aa_rating_id' => $ratingId,
241276 'aa_rating_value' => $ratingValue,
242277 'aa_design_bucket' => $bucket,
 278+ ),
 279+ __METHOD__,
 280+ array( 'IGNORE' )
 281+ );
 282+
 283+ if ( !$dbw->affectedRows() ) {
 284+ $dbw->update(
 285+ 'article_feedback',
 286+ array(
 287+ 'aa_timestamp' => $timestamp,
 288+ 'aa_rating_value' => $ratingValue,
 289+ ),
 290+ array(
 291+ 'aa_page_id' => $pageId,
 292+ 'aa_user_text' => $user->getName(),
 293+ 'aa_revision' => $revisionId,
 294+ 'aa_rating_id' => $ratingId,
 295+ 'aa_user_anon_token' => $token,
 296+ ),
 297+ __METHOD__
243298 );
244299 }
245 -
246 - // In MySQL, there is native support for multi-row inserts and our rows
247 - // will automatically get consecutive insertIDs. In SQLite this seems
248 - // to be the case if you use a transaction, but I couldn't find anything
249 - // on the web that states whether this is true.
250 - $dbw->begin();
251 - $dbw->insert( 'article_feedback', $rows, __METHOD__ );
252 - $dbw->commit();
253 -
254 - return $dbw->insertID();
255300 }
256301
257302 /**
Index: trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
@@ -179,13 +179,14 @@
180180 array(
181181 'aa_page_id' => $params['pageid'],
182182 'aa_rating_id' => array_keys( $wgArticleFeedbackRatingTypes ),
 183+ 'aa_user_id' => $wgUser->getId(),
183184 'aa_user_text' => $wgUser->getName(),
184185 'aa_user_anon_token' => $this->getAnonToken( $params ),
185186 ),
186187 __METHOD__,
187188 array(
188189 'LIMIT' => count( $wgArticleFeedbackRatingTypes ),
189 - 'ORDER BY' => 'aa_id DESC',
 190+ 'ORDER BY' => 'aa_revision DESC',
190191 )
191192 );
192193 $ratings = array();

Follow-up revisions

RevisionCommit summaryAuthorDate
r110962Followup r110959: reapply the good parts of r96722:...catrope20:52, 8 February 2012
r110963Followup r110959: also remove the schema change in the hooks filecatrope20:56, 8 February 2012
r110967Followup r110959: reintroduce another good part of r96722 (sort of): add an i...catrope21:44, 8 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r96722Epic followup to r94404, r94509: change the ArticleFeedback schema such that ...catrope15:26, 10 September 2011

Status & tagging log