r84535 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r84534‎ \| r84535 \| r84536 >
Date:	17:18, 22 March 2011
Author:	tparscal
Status:	reverted (Comments)
Tags:
Comment:	Step 2 of 2 for implementing expirations. * Added revision column to page table which is part of the primary key * Modified ApiQueryArticleFeedback to sum up counts and totals for all valid revisions * Modified ApiArticleFeedback to automatically subtract totals and counts from previous revision's tally
Modified paths:	/trunk/extensions/ArticleFeedback/ArticleFeedback.hooks.php (modified) (history) /trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php (modified) (history) /trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php (modified) (history) /trunk/extensions/ArticleFeedback/sql/AddPageRevisionColumn.sql (added) (history) /trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedback/api/ApiArticleFeedback.php
—	—	@@ -45,7 +45,8 @@
46	46	$lastRatings = array();
47	47
48	48	foreach ( $res as $row ) {
49		~~- $lastRatings[$row->aa_rating_id] = $row->aa_rating_value;~~
	49	+ $lastRatings[$row->aa_rating_id]['value'] = $row->aa_rating_value;
	50	+ $lastRatings[$row->aa_rating_id]['revision'] = $row->aa_revision;
50	51	}
51	52
52	53	$pageId = $params['pageid'];
—	—	@@ -54,7 +55,8 @@
55	56	foreach( $wgArticleFeedbackRatings as $rating ) {
56	57	$lastRating = false;
57	58	if ( isset( $lastRatings[$rating] ) ) {
58		~~- $lastRating = intval( $lastRatings[$rating] );~~
	59	+ $lastRating = intval( $lastRatings[$rating]['value'] );
	60	+ $lastRevision = intval( $lastRatings[$rating]['revision'] );
59	61	}
60	62
61	63	$thisRating = false;
—	—	@@ -62,7 +64,7 @@
63	65	$thisRating = intval( $params["r{$rating}"] );
64	66	}
65	67
66		~~- $this->insertPageRating( $pageId, $rating, ( $thisRating - $lastRating ),~~
	68	+ $this->insertPageRating( $pageId, $revisionId, $lastRevision, $rating, ( $thisRating - $lastRating ),
67	69	$thisRating, $lastRating
68	70	);
69	71
—	—	@@ -79,32 +81,15 @@
80	82	* Inserts (or Updates, where appropriate) the aggregate page rating
81	83	*
82	84	* @param $pageId Integer: Page Id
	85	+ * @param $revisionId Integer: Revision Id
83	86	* @param $ratingId Integer: Rating Id
84	87	* @param $updateAddition Integer: Difference between user's last rating (if applicable)
85	88	* @param $thisRating Integer\|Boolean: Value of the Rating
86	89	* @param $lastRating Integer\|Boolean: Value of the last Rating
87	90	*/
88		~~- private function insertPageRating( $pageId, $ratingId, $updateAddition, $thisRating, $lastRating ) {~~
	91	+ private function insertPageRating( $pageId, $revisionId, $lastRevision, $ratingId, $updateAddition, $thisRating, $lastRating ) {
89	92	$dbw = wfGetDB( DB_MASTER );
90	93
91		~~- // 0 == No change in rating count~~
92		~~- // 1 == No rating last time (or new rating), and now there is~~
93		~~- // -1 == Rating last time, but abstained this time~~
94		~~- $countChange = 0;~~
95		~~- if ( $lastRating === false \|\| $lastRating === 0 ) {~~
96		~~- if ( $thisRating === 0 ) {~~
97		~~- $countChange = 0;~~
98		~~- } else {~~
99		~~- $countChange = 1;~~
100		~~- }~~
101		~~- } else { // Last rating was > 0~~
102		~~- if ( $thisRating === 0 ) {~~
103		~~- $countChange = -1;~~
104		~~- } else {~~
105		~~- $countChange = 0;~~
106		~~- }~~
107		~~- }~~
108		-
109	94	$dbw->insert(
110	95	'article_feedback_pages',
111	96	array(
—	—	@@ -112,23 +97,73 @@
113	98	'aap_total' => 0,
114	99	'aap_count' => 0,
115	100	'aap_rating_id' => $ratingId,
	101	+ 'aap_revision' => $revisionId,
116	102	),
117	103	__METHOD__,
118	104	array( 'IGNORE' )
119	105	);
120	106
121		~~- $dbw->update(~~
122		~~- 'article_feedback_pages',~~
123		~~- array(~~
124		~~- 'aap_total = aap_total + ' . $updateAddition,~~
125		~~- 'aap_count = aap_count + ' . $countChange,~~
126		~~- ),~~
127		~~- array(~~
128		~~- 'aap_page_id' => $pageId,~~
129		~~- 'aap_rating_id' => $ratingId,~~
130		~~- ),~~
131		~~- __METHOD__~~
132		~~- );~~
	107	+ if ( $dbw->affectedRows() ) {
	108	+ if ( $lastRating ) {
	109	+ $dbw->update(
	110	+ 'article_feedback_pages',
	111	+ array(
	112	+ 'aap_total = aap_total - ' . intval( $lastRating ),
	113	+ 'aap_count = aap_count - 1',
	114	+ ),
	115	+ array(
	116	+ 'aap_page_id' => $pageId,
	117	+ 'aap_rating_id' => $ratingId,
	118	+ 'aap_revision' => $lastRevision
	119	+ ),
	120	+ __METHOD__
	121	+ );
	122	+ }
	123	+ $dbw->update(
	124	+ 'article_feedback_pages',
	125	+ array(
	126	+ 'aap_total' => $thisRating,
	127	+ 'aap_count' => 1,
	128	+ ),
	129	+ array(
	130	+ 'aap_page_id' => $pageId,
	131	+ 'aap_rating_id' => $ratingId,
	132	+ 'aap_revision' => $revisionId,
	133	+ ),
	134	+ __METHOD__
	135	+ );
	136	+ } else {
	137	+ // 0 == No change in rating count
	138	+ // 1 == No rating last time (or new rating), and now there is
	139	+ // -1 == Rating last time, but abstained this time
	140	+ $countChange = 0;
	141	+ if ( $lastRating === false \|\| $lastRating === 0 ) {
	142	+ if ( $thisRating === 0 ) {
	143	+ $countChange = 0;
	144	+ } else {
	145	+ $countChange = 1;
	146	+ }
	147	+ } else { // Last rating was > 0
	148	+ if ( $thisRating === 0 ) {
	149	+ $countChange = -1;
	150	+ } else {
	151	+ $countChange = 0;
	152	+ }
	153	+ }
	154	+ $dbw->update(
	155	+ 'article_feedback_pages',
	156	+ array(
	157	+ 'aap_total = aap_total + ' . $updateAddition,
	158	+ 'aap_count = aap_count + ' . $countChange,
	159	+ ),
	160	+ array(
	161	+ 'aap_page_id' => $pageId,
	162	+ 'aap_rating_id' => $ratingId,
	163	+ 'aap_revision' => $revisionId,
	164	+ ),
	165	+ __METHOD__
	166	+ );
	167	+ }
133	168	}
134	169
135	170	/**
—	—	@@ -153,12 +188,12 @@
154	189	'aa_page_id' => $pageId,
155	190	'aa_user_id' => $user->getId(),
156	191	'aa_user_text' => $user->getName(),
	192	+ 'aa_user_anon_token' => $token,
157	193	'aa_revision' => $revisionId,
158	194	'aa_timestamp' => $timestamp,
159	195	'aa_rating_id' => $ratingId,
160	196	'aa_rating_value' => $ratingValue,
161	197	'aa_design_bucket' => $bucket,
162		~~- 'aa_user_anon_token' => $token~~
163	198	),
164	199	__METHOD__,
165	200	array( 'IGNORE' )
—	—	@@ -176,7 +211,7 @@
177	212	'aa_user_text' => $user->getName(),
178	213	'aa_revision' => $revisionId,
179	214	'aa_rating_id' => $ratingId,
180		~~- 'aa_user_anon_token' => $token~~
	215	+ 'aa_user_anon_token' => $token,
181	216	),
182	217	__METHOD__
183	218	);
Index: trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
—	—	@@ -10,9 +10,13 @@
11	11
12	12	$result = $this->getResult();
13	13
	14	+ $revisionLimit = self::getRevisionLimit( $params['pageid'] );
	15	+
14	16	$this->addTables( array( 'article_feedback_pages', 'article_feedback_ratings' ) );
15	17
16		~~- $this->addFields( array( 'aap_page_id', 'aap_total', 'aap_count', 'aap_rating_id', 'aar_rating' ) );~~
	18	+ $this->addFields( array(
	19	+ 'aap_page_id', 'SUM(aap_total) as aap_total', 'SUM(aap_count) as aap_count', 'aap_rating_id', 'aar_rating'
	20	+ ) );
17	21
18	22	$this->addJoinConds( array(
19	23	'article_feedback_ratings' => array( 'LEFT JOIN', array(
—	—	@@ -23,7 +27,11 @@
24	28	) );
25	29
26	30	$this->addWhereFld( 'aap_page_id', $params['pageid'] );
	31	+ $this->addWhereFld( 'aap_revision > ' . $revisionLimit );
27	32
	33	+ $this->addOption( 'GROUP BY', 'aap_rating_id' );
	34	+ $this->addOption( 'LIMIT', count( $wgArticleFeedbackRatings ) );
	35	+
28	36	if ( $params['userrating'] ) {
29	37	global $wgUser;
30	38
—	—	@@ -43,7 +51,6 @@
44	52	} elseif ( strlen( $params['anontoken'] ) != 32 ) {
45	53	$this->dieUsage( 'The anontoken is not 32 characters', 'invalidtoken' );
46	54	}
47		-
48	55	$leftJoinCondsAF['aa_user_anon_token'] = $params['anontoken'];
49	56	} else {
50	57	$leftJoinCondsAF['aa_user_anon_token'] = '';
—	—	@@ -61,8 +68,6 @@
62	69	$this->addOption( 'ORDER BY', 'aa_revision DESC' );
63	70	}
64	71
65		~~- $this->addOption( 'LIMIT', count( $wgArticleFeedbackRatings ) );~~
66		-
67	72	$res = $this->select( __METHOD__ );
68	73
69	74	$ratings = array();
—	—	@@ -106,13 +111,12 @@
107	112	// Ratings can only be expired if the user has rated before
108	113	$ratings[$params['pageid']]['status'] = 'current';
109	114	if ( $params['userrating'] && $userRatedArticle ) {
110		~~- $dbr = wfGetDb( DB_SLAVE );~~
111		-
112	115	global $wgArticleFeedbackRatingLifetime;
113	116
114		~~- $res = $dbr->select(~~
	117	+ $dbr = wfGetDb( DB_SLAVE );
	118	+ $updates = $dbr->selectField(
115	119	'revision',
116		~~- 'rev_id',~~
	120	+ 'COUNT(rev_id) as revisions',
117	121	array(
118	122	'rev_page' => $params['pageid'],
119	123	'rev_id > ' . $ratings[$pageId]['revid']
—	—	@@ -120,13 +124,12 @@
121	125	__METHOD__,
122	126	array ( 'LIMIT', $wgArticleFeedbackStaleCount + 1 )
123	127	);
124		-
125		~~- if ( $res && $dbr->numRows( $res ) > $wgArticleFeedbackRatingLifetime ) {~~
	128	+ if ( $updates > $wgArticleFeedbackRatingLifetime ) {
126	129	// Expired status
127	130	$ratings[$params['pageid']]['status'] = 'expired';
128	131	}
129	132	}
130		-
	133	+
131	134	foreach ( $ratings as $rat ) {
132	135	$result->setIndexedTagName( $rat['ratings'], 'r' );
133	136	$result->addValue( array( 'query', $this->getModuleName() ), null, $rat );
—	—	@@ -135,6 +138,33 @@
136	139	$result->setIndexedTagName_internal( array( 'query', $this->getModuleName() ), 'aa' );
137	140	}
138	141
	142	+ /**
	143	+ * Get the revision number of the oldest revision still being counted in totals.
	144	+ *
	145	+ * @param $pageId Integer: ID of page to check revisions for
	146	+ * @return Integer: Oldest valid revision number or 0 of all revisions are valid
	147	+ */
	148	+ protected static function getRevisionLimit( $pageId ) {
	149	+ global $wgArticleFeedbackRatingLifetime;
	150	+
	151	+ $dbr = wfGetDb( DB_SLAVE );
	152	+ $revision = $dbr->selectField(
	153	+ 'revision',
	154	+ 'rev_id',
	155	+ array( 'rev_page' => $pageId ),
	156	+ __METHOD__,
	157	+ array(
	158	+ 'ORDER BY' => 'rev_id DESC',
	159	+ 'LIMIT' => 1,
	160	+ 'OFFSET' => $wgArticleFeedbackRatingLifetime - 1
	161	+ )
	162	+ );
	163	+ if ( $revision ) {
	164	+ return intval( $revision );
	165	+ }
	166	+ return 0;
	167	+ }
	168	+
139	169	public function getAllowedParams() {
140	170	return array(
141	171	'pageid' => array(
Index: trunk/extensions/ArticleFeedback/sql/AddPageRevisionColumn.sql
—	—	@@ -0,0 +1,4 @@
	2	+ALTER TABLE /_/article_feedback_pages
	3	+ ADD aap_revision integer unsigned NOT NULL,
	4	+ DROP PRIMARY KEY,
	5	+ ADD PRIMARY KEY (aap_page_id, aap_rating_id, aap_revision);
\ No newline at end of file
Index: trunk/extensions/ArticleFeedback/sql/ArticleFeedback.sql
—	—	@@ -34,12 +34,15 @@
35	35	-- 1 vote per user per revision
36	36	PRIMARY KEY (aa_revision, aa_user_text, aa_rating_id, aa_user_anon_token)
37	37	) /$wgDBTableOptions/;
38		~~-CREATE INDEX /i/aa_user_page_revision ON /_/article_feedback (aa_user_id, aa_page_id, aa_revision);~~
	38	+CREATE INDEX /i/aa_user_page_revision ON /_/article_feedback
	39	+ (aa_user_id, aa_page_id, aa_revision);
39	40
40	41	-- Aggregate rating table for a page
41	42	CREATE TABLE IF NOT EXISTS /_/article_feedback_pages (
42	43	-- Foreign key to page.page_id
43	44	aap_page_id integer unsigned NOT NULL,
	45	+ -- Revision that totals are relevant to
	46	+ aap_revision integer unsigned NOT NULL,
44	47	-- Foreign key to article_feedback_ratings.aar_rating
45	48	aap_rating_id integer unsigned NOT NULL,
46	49	-- Sum (total) of all the ratings for this article revision
—	—	@@ -47,7 +50,7 @@
48	51	-- Number of ratings
49	52	aap_count integer unsigned NOT NULL,
50	53	-- One rating row per page
51		~~- PRIMARY KEY (aap_page_id, aap_rating_id)~~
	54	+ PRIMARY KEY (aap_page_id, aap_rating_id, aap_revision)
52	55	) /$wgDBTableOptions/;
53	56
54	57	-- Properties table for meta information
—	—	@@ -57,7 +60,6 @@
58	61	afp_revision integer unsigned NOT NULL,
59	62	afp_user_text varbinary(255) NOT NULL,
60	63	afp_user_anon_token varbinary(32) NOT NULL DEFAULT '',
61		-
62	64	-- Key/value pairs
63	65	afp_key varbinary(255) NOT NULL,
64	66	-- Integer value
—	—	@@ -65,4 +67,5 @@
66	68	-- Text value
67	69	afp_value_text varbinary(255) DEFAULT '' NOT NULL
68	70	) /$wgDBTableOptions/;
69		~~-CREATE UNIQUE INDEX /i/afp_rating_key ON /_/article_feedback_properties (afp_revision, afp_user_text, afp_user_anon_token, afp_key);~~
	71	+CREATE UNIQUE INDEX /i/afp_rating_key ON /_/article_feedback_properties
	72	+ (afp_revision, afp_user_text, afp_user_anon_token, afp_key);
Index: trunk/extensions/ArticleFeedback/ArticleFeedback.hooks.php
—	—	@@ -137,6 +137,15 @@
138	138	true
139	139	) );
140	140	}
	141	+ if ( !$db->fieldExists( 'article_feedback_pages', 'aap_revision', __METHOD__ ) ) {
	142	+ $updater->addExtensionUpdate( array(
	143	+ 'addField',
	144	+ 'article_feedback_pages',
	145	+ 'aap_revision',
	146	+ $dir . '/sql/AddPageRevisionColumn.sql',
	147	+ true
	148	+ ) );
	149	+ }
141	150	$updater->addExtensionUpdate( array(
142	151	'addTable',
143	152	'article_feedback_properties',
—	—	@@ -156,7 +165,7 @@
157	166	}
158	167	return true;
159	168	}
160		-
	169	+
161	170	/**
162	171	* ParserTestTables hook
163	172	*/

Follow-up revisions

Revision	Commit summary	Author	Date
r84537	Added documentation for parameter added to insertPageRating in r84535.	tparscal	17:24, 22 March 2011
r84623	Added comments to help make it easier to understand the program flow - see r8...	tparscal	18:57, 23 March 2011
r84627	Fixes issues in r84535...	tparscal	19:28, 23 March 2011
r84629	Added comments to $pageId and $rev variables within a loop to help improve co...	tparscal	19:45, 23 March 2011
r84637	Fixes more issues in r84535...	tparscal	21:52, 23 March 2011
r85964	ArticleFeedback: Revert most of r84535, removing aap_revision again. This wil...	catrope	17:58, 13 April 2011

Comments

#Comment by Catrope (talk | contribs) 20:18, 22 March 2011

The logic in ApiArticleFeedback::insertPageRating() looks a bit wrong. Basically it looks like this:

try to insert article_feedback_pages row
if ( it wasn't there yet ) {
	if ( there was a previous rating ) { // how can that even happen? There was no pre-existing afp row!
		subtract the last rating from aap_total // how? That row doesn't exist, remember?
		subtract one from aap_count // same
	}
	update the afp table to set count to 1
		and rating to the current rating // can't do this with update, I guess you mean insert. Also overwrites the updates above, if they hadn't been impossible in the first place
} else {
	update the rating and count in the existing row as appropriate // this part is fine
}

#Comment by Catrope (talk | contribs) 21:27, 22 March 2011

Rereading the code, it turns out it all makes complete sense. I must've been smoking something.

#Comment by Catrope (talk | contribs) 20:26, 22 March 2011

-			$res = $dbr->select(
+			$dbr = wfGetDb( DB_SLAVE );
+			$updates = $dbr->selectField(
 				'revision',
-				'rev_id',
+				'COUNT(rev_id) as revisions',
 				array(
 					'rev_page' => $params['pageid'],
 					'rev_id > ' . $ratings[$pageId]['revid']
@@ -120,13 +124,12 @@
 				__METHOD__,
 				array ( 'LIMIT', $wgArticleFeedbackStaleCount + 1 )
 			);
-
-			if ( $res && $dbr->numRows( $res ) > $wgArticleFeedbackRatingLifetime ) {
+			if ( $updates > $wgArticleFeedbackRatingLifetime ) {

This checks whether the old rating is stale by counting how many revisions there were between the current revision and the old one. This is unacceptable, since the number of revisions in between may be arbitrarily large. Also, LIMIT doesn't work the way you think it does when combined with COUNT(). You don't even need this stuff either, since you already have getRevisionLimit(): use it.

Status & tagging log

11:06, 5 May 2011 Catrope (talk | contribs) changed the tags for r84535 [removed: aftdeploy]
18:00, 13 April 2011 Catrope (talk | contribs) changed the status of r84535 [removed: resolved added: reverted]
17:06, 7 April 2011 Trevor Parscal (WMF) (talk | contribs) changed the tags for r84535 [removed: roan]
19:34, 4 April 2011 Catrope (talk | contribs) changed the tags for r84535 [added: aftdeploy]
15:45, 28 March 2011 Catrope (talk | contribs) changed the status of r84535 [removed: new added: resolved]
21:55, 23 March 2011 Trevor Parscal (WMF) (talk | contribs) changed the tags for r84535 [added: roan]
21:55, 23 March 2011 Trevor Parscal (WMF) (talk | contribs) changed the status of r84535 [removed: fixme added: new]
20:26, 22 March 2011 Catrope (talk | contribs) changed the status of r84535 [removed: new added: fixme]