r84627 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r84626‎ \| r84627 \| r84628 >
Date:	19:28, 23 March 2011
Author:	tparscal
Status:	resolved (Comments)
Tags:
Comment:	Fixes issues in r84535 * Properly summing total rows for valid revisions * Moved user rating query to it's own method
Modified paths:	/trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedback/api/ApiQueryArticleFeedback.php
—	—	@@ -6,18 +6,20 @@
7	7
8	8	public function execute() {
9	9	global $wgArticleFeedbackRatings;
	10	+
10	11	$params = $this->extractRequestParams();
11		-
12	12	$result = $this->getResult();
13		-
14		~~- $revisionLimit = self::getRevisionLimit( $params['pageid'] );~~
15		-
	13	+ $revisionLimit = $this->getRevisionLimit( $params['pageid'] );
	14	+
16	15	$this->addTables( array( 'article_feedback_pages', 'article_feedback_ratings' ) );
17		-
18	16	$this->addFields( array(
19		~~- 'aap_page_id', 'SUM(aap_total) as aap_total', 'SUM(aap_count) as aap_count', 'aap_rating_id', 'aar_rating'~~
	17	+ 'aap_page_id',
	18	+ 'MAX(aap_revision) as aap_revision',
	19	+ 'SUM(aap_total) as aap_total',
	20	+ 'SUM(aap_count) as aap_count',
	21	+ 'aap_rating_id',
	22	+ 'aar_rating',
20	23	) );
21		-
22	24	$this->addJoinConds( array(
23	25	'article_feedback_ratings' => array( 'LEFT JOIN', array(
24	26	'aar_id=aap_rating_id',
—	—	@@ -25,67 +27,38 @@
26	28	)
27	29	),
28	30	) );
29		-
30	31	$this->addWhereFld( 'aap_page_id', $params['pageid'] );
31		~~- $this->addWhereFld( 'aap_revision > ' . $revisionLimit );~~
32		-
	32	+ $this->addWhere( 'aap_revision >= ' . $revisionLimit );
33	33	$this->addOption( 'GROUP BY', 'aap_rating_id' );
34	34	$this->addOption( 'LIMIT', count( $wgArticleFeedbackRatings ) );
35	35
	36	+ $res = $this->select( __METHOD__ );
	37	+
36	38	if ( $params['userrating'] ) {
37		~~- global $wgUser;~~
38		-
39		~~- $leftJoinCondsAF = array(~~
40		~~- 'aa_page_id=aap_page_id',~~
41		~~- 'aa_rating_id=aap_rating_id',~~
42		~~- 'aa_user_id=' . $wgUser->getId() );~~
43		~~- $leftJoinCondsAFP = array(~~
44		~~- 'afp_revision=aa_revision',~~
45		~~- 'afp_user_text=aa_user_text',~~
46		~~- 'afp_user_anon_token=aa_user_anon_token',~~
47		~~- 'afp_key' => 'expertise' );~~
48		-
49		~~- if ( $wgUser->isAnon() ) {~~
50		~~- if ( !isset( $params['anontoken'] ) ) {~~
51		~~- $this->dieUsageMsg( array( 'missingparam', 'anontoken' ) );~~
52		~~- } elseif ( strlen( $params['anontoken'] ) != 32 ) {~~
53		~~- $this->dieUsage( 'The anontoken is not 32 characters', 'invalidtoken' );~~
54		~~- }~~
55		~~- $leftJoinCondsAF['aa_user_anon_token'] = $params['anontoken'];~~
56		~~- } else {~~
57		~~- $leftJoinCondsAF['aa_user_anon_token'] = '';~~
58		~~- }~~
59		-
60		~~- $this->addTables( array( 'article_feedback', 'article_feedback_properties' ) );~~
61		~~- $this->addJoinConds( array(~~
62		~~- 'article_feedback' => array( 'LEFT JOIN', $leftJoinCondsAF ),~~
63		~~- 'article_feedback_properties' => array( 'LEFT JOIN', $leftJoinCondsAFP )~~
64		~~- )~~
65		~~- );~~
66		-
67		~~- $this->addFields( array( 'aa_rating_value', 'aa_revision', 'afp_value_text' ) );~~
68		-
69		~~- $this->addOption( 'ORDER BY', 'aa_revision DESC' );~~
	39	+ $userRatings = $this->getUserRatings( $params );
70	40	}
71	41
72		~~- $res = $this->select( __METHOD__ );~~
73		-
74	42	$ratings = array();
75	43
76	44	$userRatedArticle = false;
77	45
78		~~- foreach ( $res as $row ) {~~
	46	+ $rev = null;
	47	+ foreach ( $res as $i => $row ) {
79	48	$pageId = $row->aap_page_id;
80	49
	50	+ if ( is_null( $rev ) ) {
	51	+ $rev = $row->aap_revision;
	52	+ }
	53	+
81	54	if ( !isset( $ratings[$pageId] ) ) {
82	55	$page = array(
83	56	'pageid' => $pageId,
	57	+ 'revid' => $rev,
84	58	);
85	59
86	60	if ( $params['userrating'] ) {
87		~~- $page['revid'] = $row->aa_revision;~~
88		~~- if ( !is_null( $row->afp_value_text ) ) {~~
89		~~- $page['expertise'] = $row->afp_value_text;~~
	61	+ if ( isset( $userRatings[$i]['expertise'] ) ) {
	62	+ $page['expertise'] = $userRatings[$i]['expertise'];
90	63	}
91	64	}
92	65
—	—	@@ -99,8 +72,8 @@
100	73	'count' => $row->aap_count,
101	74	);
102	75
103		~~- if ( $params['userrating'] && !is_null( $row->aa_rating_value ) ) {~~
104		~~- $thisRow['userrating'] = $row->aa_rating_value;~~
	76	+ if ( $params['userrating'] && isset( $userRatings[$i]['value'] ) ) {
	77	+ $thisRow['userrating'] = $userRatings[$i]['value'];
105	78
106	79	$userRatedArticle = true;
107	80	}
—	—	@@ -109,25 +82,8 @@
110	83	}
111	84
112	85	// Ratings can only be expired if the user has rated before
113		~~- $ratings[$params['pageid']]['status'] = 'current';~~
114	86	if ( $params['userrating'] && $userRatedArticle ) {
115		~~- global $wgArticleFeedbackRatingLifetime;~~
116		-
117		~~- $dbr = wfGetDb( DB_SLAVE );~~
118		~~- $updates = $dbr->selectField(~~
119		~~- 'revision',~~
120		~~- 'COUNT(rev_id) as revisions',~~
121		~~- array(~~
122		~~- 'rev_page' => $params['pageid'],~~
123		~~- 'rev_id > ' . $ratings[$pageId]['revid']~~
124		~~- ),~~
125		~~- __METHOD__,~~
126		~~- array ( 'LIMIT', $wgArticleFeedbackStaleCount + 1 )~~
127		~~- );~~
128		~~- if ( $updates > $wgArticleFeedbackRatingLifetime ) {~~
129		~~- // Expired status~~
130		~~- $ratings[$params['pageid']]['status'] = 'expired';~~
131		~~- }~~
	87	+ $ratings[$params['pageid']]['status'] = $rev >= $revisionLimit ? 'current' : 'expired';
132	88	}
133	89
134	90	foreach ( $ratings as $rat ) {
—	—	@@ -138,13 +94,61 @@
139	95	$result->setIndexedTagName_internal( array( 'query', $this->getModuleName() ), 'aa' );
140	96	}
141	97
	98	+ protected function getUserRatings( $params ) {
	99	+ global $wgUser, $wgArticleFeedbackRatings;
	100	+
	101	+ $pageId = $params['pageid'];
	102	+ $token = '';
	103	+ if ( $wgUser->isAnon() ) {
	104	+ if ( !isset( $params['anontoken'] ) ) {
	105	+ $this->dieUsageMsg( array( 'missingparam', 'anontoken' ) );
	106	+ } elseif ( strlen( $params['anontoken'] ) != 32 ) {
	107	+ $this->dieUsage( 'The anontoken is not 32 characters', 'invalidtoken' );
	108	+ }
	109	+ $token = $params['anontoken'];
	110	+ }
	111	+
	112	+ $dbr = wfGetDb( DB_SLAVE );
	113	+ $res = $dbr->select(
	114	+ array( 'article_feedback', 'article_feedback_properties' ),
	115	+ array( 'aa_rating_id', 'aa_rating_value', 'afp_value_text' ),
	116	+ array(
	117	+ 'aa_page_id' => $pageId,
	118	+ 'aa_rating_id' => $wgArticleFeedbackRatings,
	119	+ 'aa_user_id' => $wgUser->getId(),
	120	+ ),
	121	+ __METHOD__,
	122	+ array(
	123	+ 'ORDER BY' => 'aa_revision DESC',
	124	+ 'LIMIT' => count( $wgArticleFeedbackRatings )
	125	+ ),
	126	+ array( 'article_feedback_properties' => array(
	127	+ 'LEFT JOIN', array(
	128	+ 'afp_revision=aa_revision',
	129	+ 'afp_user_text=aa_user_text',
	130	+ 'afp_user_anon_token=aa_user_anon_token',
	131	+ 'aa_user_anon_token' => $token,
	132	+ 'afp_key' => 'expertise',
	133	+ )
	134	+ ) )
	135	+ );
	136	+ $ratings = array();
	137	+ foreach ( $res as $row ) {
	138	+ $ratings[$row->aa_rating_id] = array(
	139	+ 'value' => $row->aa_rating_value,
	140	+ 'expertise' => $row->afp_value_text
	141	+ );
	142	+ }
	143	+ return $ratings;
	144	+ }
	145	+
142	146	/**
143	147	* Get the revision number of the oldest revision still being counted in totals.
144	148	*
145	149	* @param $pageId Integer: ID of page to check revisions for
146	150	* @return Integer: Oldest valid revision number or 0 of all revisions are valid
147	151	*/
148		~~- protected static function getRevisionLimit( $pageId ) {~~
	152	+ protected function getRevisionLimit( $pageId ) {
149	153	global $wgArticleFeedbackRatingLifetime;
150	154
151	155	$dbr = wfGetDb( DB_SLAVE );

Follow-up revisions

Revision	Commit summary	Author	Date
r84929	Resolved issues with r84627 where incomplete ratings sets (a missing user rat...	tparscal	21:47, 28 March 2011

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r84535	Step 2 of 2 for implementing expirations....	tparscal	17:18, 22 March 2011

Comments

#Comment by Catrope (talk | contribs) 15:51, 28 March 2011

+		foreach ( $res as $row ) {
+			$ratings[$row->aa_rating_id] = array(
+				'value' => $row->aa_rating_value,
+				'expertise' => $row->afp_value_text
+			);
+

You need to be a bit more careful here. You're essentially doing SELECT ... WHERE aa_rating_id IN (1,2,3,4) .... LIMIT 4 and expecting one of each rating type. You'll get this as long as the latest rating is complete (has rows for all 4 types); if it's not, for whatever reason (DB corruption, change in $wgArticleFeedbackRatings), this loop overwrites recent ratings with old ones, resulting in incorrect data being returned.

You should store the aa_revision value of the first row you process, then only process rows with matching aa_revision values.

OK otherwise.

Status & tagging log

11:06, 5 May 2011 Catrope (talk | contribs) changed the tags for r84627 [removed: aftdeploy]
17:06, 7 April 2011 Trevor Parscal (WMF) (talk | contribs) changed the tags for r84627 [removed: roan]
19:34, 4 April 2011 Catrope (talk | contribs) changed the tags for r84627 [added: aftdeploy]
18:48, 4 April 2011 Catrope (talk | contribs) changed the status of r84627 [removed: new added: resolved]
19:04, 1 April 2011 Trevor Parscal (WMF) (talk | contribs) changed the status of r84627 [removed: fixme added: new]
15:51, 28 March 2011 Catrope (talk | contribs) changed the status of r84627 [removed: new added: fixme]
21:03, 24 March 2011 Trevor Parscal (WMF) (talk | contribs) changed the tags for r84627 [added: roan]