r92317 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r92316‎ \| r92317 \| r92318 >
Date:	21:53, 15 July 2011
Author:	reedy
Status:	deferred
Tags:
Comment:	REL1_18 FR: r87606, r87617, r87669, r87690, r87700, r90388, r90419, r90741, r90789, r90793, r91363, r91365, r91584
Modified paths:	/branches/REL1_18/extensions/FlaggedRevs (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/api/FlaggedRevsApi.hooks.php (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/business/RevisionReviewForm.php (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/dataclasses/FlaggedRevision.php (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/presentation/FlaggedPageView.php (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/presentation/FlaggedRevsUI.hooks.php (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/presentation/modules/flaggedrevs.css (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/presentation/specialpages/reports/ReviewedVersions_body.php (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/schema/FlaggedRevsUpdater.hooks.php (modified) (history) /branches/REL1_18/extensions/FlaggedRevs/tests/FRUserCountersTest.php (modified) (history)

Diff [purge]

Index: branches/REL1_18/extensions/FlaggedRevs/schema/FlaggedRevsUpdater.hooks.php
—	—	@@ -33,8 +33,8 @@
34	34	'flaggedrevs_stats', "$base/patch-flaggedrevs_stats.sql", true ) );
35	35	$du->addExtensionUpdate( array( 'FlaggedRevsUpdaterHooks::doFlaggedImagesTimestampNULL',
36	36	"$base/patch-fi_img_timestamp.sql" ) );
37		~~- $du->addExtensionUpdate( array( 'addIndex',~~
38		~~- 'flaggedrevs', 'page_rev', "$base/patch-fr_page_rev-index.sql", true ) );~~
	37	+ $du->addExtensionUpdate( array( 'FlaggedRevsUpdaterHooks::doFlaggedRevsRevTimestamp',
	38	+ "$base/patch-fr_page_rev-index.sql" ) );
39	39	} elseif ( $wgDBtype == 'postgres' ) {
40	40	$base = dirname( __FILE__ ) . '/postgres';
41	41	// Initial install tables (current schema)
—	—	@@ -62,8 +62,8 @@
63	63	// @TODO: PG stats table???
64	64	$du->addExtensionUpdate( array( 'FlaggedRevsUpdaterHooks::doFlaggedImagesTimestampNULL',
65	65	"$base/patch-fi_img_timestamp.sql" ) );
66		~~- $du->addExtensionUpdate( array( 'addIndex',~~
67		~~- 'flaggedrevs', 'page_rev', "$base/patch-fr_page_rev-index.sql", true ) );~~
	66	+ $du->addExtensionUpdate( array( 'FlaggedRevsUpdaterHooks::doFlaggedRevsRevTimestamp',
	67	+ "$base/patch-fr_page_rev-index.sql" ) );
68	68	} elseif ( $wgDBtype == 'sqlite' ) {
69	69	$base = dirname( __FILE__ ) . '/mysql';
70	70	$du->addExtensionUpdate( array( 'addTable',
—	—	@@ -82,4 +82,21 @@
83	83	$du->getDB()->sourceFile( $patch );
84	84	$du->output( "done.\n" );
85	85	}
	86	+
	87	+ public static function doFlaggedRevsRevTimestamp( $du, $patch ) {
	88	+ $exists = $du->getDB()->fieldInfo( 'flaggedrevs', 'fr_rev_timestamp' );
	89	+ if ( $exists ) {
	90	+ $du->output( "...fr_rev_timestamp already exists.\n" );
	91	+ return;
	92	+ }
	93	+ include_once( dirname( __FILE__ ) . "/../maintenance/populateRevTimestamp.inc" );
	94	+ if ( !function_exists( 'populate_fr_rev_timestamp' ) ) {
	95	+ $du->output( "...populateRevTimestamp.inc missing! Aborting fr_rev_timestamp update.\n" );
	96	+ return; // sanity
	97	+ }
	98	+ $du->output( "Adding fr_rev_timestamp and redoing flaggedrevs table indexes... " );
	99	+ $du->getDB()->sourceFile( $patch );
	100	+ populate_fr_rev_timestamp( 0 );
	101	+ $du->output( "done.\n" );
	102	+ }
86	103	}
Index: branches/REL1_18/extensions/FlaggedRevs/tests/FRUserCountersTest.php
—	—	@@ -69,6 +69,7 @@
70	70	FRUserCounters::updateUserParams( $copyP, $article, "edit summary" );
71	71	$this->assertEquals( $p['totalContentEdits']+1, $copyP['totalContentEdits'],
72	72	"Content edit count on content edit" );
	73	+
73	74	$expected = $p['uniqueContentPages'];
74	75	$expected[] = 0;
75	76	$this->assertEquals( $expected, $copyP['uniqueContentPages'],
—	—	@@ -85,6 +86,9 @@
86	87	$copyP = $p;
87	88	FRUserCounters::updateUserParams( $copyP, $article, "/* section */" );
88	89	$this->assertEquals( $p['editComments'], $copyP['editComments'], "Auto summary" );
	90	+
	91	+ $title = Title::makeTitleSafe( 4, 'helloworld' );
	92	+ $article = new Article( $title );
89	93
90	94	$copyP = $p;
91	95	FRUserCounters::updateUserParams( $copyP, $article, "edit summary" );
Index: branches/REL1_18/extensions/FlaggedRevs/dataclasses/FlaggedRevision.php
—	—	@@ -297,7 +297,7 @@
298	298	$revRow = array(
299	299	'fr_page_id' => $this->getPage(),
300	300	'fr_rev_id' => $this->getRevId(),
301		~~- 'fr_rev_timestamp' => $this->getRevTimestamp(),~~
	301	+ 'fr_rev_timestamp' => $dbw->timestamp( $this->getRevTimestamp() ),
302	302	'fr_user' => $this->mUser,
303	303	'fr_timestamp' => $dbw->timestamp( $this->mTimestamp ),
304	304	'fr_quality' => $this->mQuality,
—	—	@@ -307,19 +307,15 @@
308	308	'fr_img_timestamp' => $dbw->timestampOrNull( $this->mFileTimestamp ),
309	309	'fr_img_sha1' => $this->mFileSha1
310	310	);
311		~~- # Update flagged revisions table~~
312		~~- $dbw->replace( 'flaggedrevs',~~
313		~~- array( array( 'fr_page_id', 'fr_rev_id' ) ), $revRow, __METHOD__ );~~
314		~~- # Clear out any previous garbage...~~
315		~~- $dbw->delete( 'flaggedtemplates',~~
316		~~- array( 'ft_rev_id' => $this->getRevId() ), __METHOD__ );~~
	311	+ # Update the main flagged revisions table...
	312	+ $dbw->insert( 'flaggedrevs', $revRow, __METHOD__, 'IGNORE' );
	313	+ if ( !$dbw->affectedRows() ) {
	314	+ return false; // duplicate review
	315	+ }
317	316	# ...and insert template version data
318	317	if ( $tmpInsertRows ) {
319	318	$dbw->insert( 'flaggedtemplates', $tmpInsertRows, __METHOD__, 'IGNORE' );
320	319	}
321		~~- # Clear out any previous garbage...~~
322		~~- $dbw->delete( 'flaggedimages',~~
323		~~- array( 'fi_rev_id' => $this->getRevId() ), __METHOD__ );~~
324	320	# ...and insert file version data
325	321	if ( $fileInsertRows ) {
326	322	$dbw->insert( 'flaggedimages', $fileInsertRows, __METHOD__, 'IGNORE' );
—	—	@@ -336,8 +332,12 @@
337	333	$dbw = wfGetDB( DB_MASTER );
338	334	# Delete from flaggedrevs table
339	335	$dbw->delete( 'flaggedrevs',
	336	+<<<<<<< .working
340	337	array( 'fr_page_id' => $this->getPage(), 'fr_rev_id' => $this->getRevId() ),
341	338	__METHOD__ );
	339	+=======
	340	+ array( 'fr_rev_id' => $this->getRevId() ), __METHOD__ );
	341	+>>>>>>> .merge-right.r87690
342	342	# Wipe versioning params...
343	343	$dbw->delete( 'flaggedtemplates',
344	344	array( 'ft_rev_id' => $this->getRevId() ), __METHOD__ );
Index: branches/REL1_18/extensions/FlaggedRevs/api/FlaggedRevsApi.hooks.php
—	—	@@ -62,7 +62,7 @@
63	63	'fr_rev_id' => array_keys( $revids ) ), LIST_AND );
64	64	}
65	65	$module->addWhere( $db->makeList( $where, LIST_OR ) );
66		~~- $module->addOption( 'USE INDEX', array( 'flaggedrevs' => 'PRIMARY' ) );~~
	66	+ //$module->addOption( 'USE INDEX', array( 'flaggedrevs' => 'page_rev' ) );
67	67
68	68	$res = $module->select( __METHOD__ );
69	69
Index: branches/REL1_18/extensions/FlaggedRevs/business/RevisionReviewForm.php
—	—	@@ -305,9 +305,17 @@
306	306	return 'review_cannot_undo';
307	307	}
308	308	$baseRevId = $newRev->isCurrent() ? $oldRev->getId() : 0;
309		~~- $article->doEdit( $new_text, $this->getComment(), 0, $baseRevId, $this->user );~~
	309	+
	310	+ # Actually make the edit...
	311	+ $editStatus = $article->doEdit(
	312	+ $new_text, $this->getComment(), 0, $baseRevId, $this->user );
	313	+
	314	+ $status = $editStatus->isOK() ? true : 'review_cannot_undo';
310	315	# If this undid one edit by another logged-in user, update user tallies
311		~~- if ( $newRev->getParentId() == $oldRev->getId() && $newRev->getRawUser() ) {~~
	316	+ if ( $status === true
	317	+ && $newRev->getParentId() == $oldRev->getId()
	318	+ && $newRev->getRawUser() )
	319	+ {
312	320	if ( $newRev->getRawUser() != $this->user->getId() ) { // no self-reverts
313	321	FRUserCounters::incCount( $newRev->getRawUser(), 'revertedEdits' );
314	322	}
—	—	@@ -367,7 +375,7 @@
368	376	return true; // don't record if the same
369	377	}
370	378
371		~~- # Insert the review entry...~~
	379	+ # The new review entry...
372	380	$flaggedRevision = new FlaggedRevision( array(
373	381	'rev' => $rev,
374	382	'user_id' => $this->user->getId(),
—	—	@@ -381,6 +389,11 @@
382	390	'fileVersions' => $fileVersions,
383	391	'flags' => ''
384	392	) );
	393	+ # Delete the old review entry if it exists...
	394	+ if ( $oldFrev ) {
	395	+ $oldFrev->delete();
	396	+ }
	397	+ # Insert the new review entry...
385	398	$flaggedRevision->insert();
386	399	# Update recent changes...
387	400	$rcId = $rev->isUnpatrolled(); // int
Index: branches/REL1_18/extensions/FlaggedRevs/presentation/FlaggedRevsUI.hooks.php
—	—	@@ -288,6 +288,7 @@
289	289	}
290	290	# Environment (e.g. $wgTitle) will change in MediaWiki::initializeArticle
291	291	if ( $clearEnvironment ) $view->clear();
	292	+
292	293	return true;
293	294	}
294	295
Index: branches/REL1_18/extensions/FlaggedRevs/presentation/specialpages/reports/ReviewedVersions_body.php
—	—	@@ -93,15 +93,15 @@
94	94	if ( !in_array( $this->namespace, $namespaces ) ) {
95	95	$conds[] = "1 = 0";
96	96	}
97		~~- $conds["fr_page_id"] = $this->pageID;~~
98		~~- $conds[] = "fr_rev_id = rev_id";~~
99		~~- $conds[] = "fr_user = user_id";~~
	97	+ $conds['fr_page_id'] = $this->pageID;
	98	+ $conds[] = 'fr_rev_id = rev_id';
100	99	$conds[] = 'rev_deleted & ' . Revision::DELETED_TEXT . ' = 0';
	100	+ $conds[] = 'fr_user = user_id';
101	101	return array(
102	102	'tables' => array( 'flaggedrevs', 'revision', 'user' ),
103	103	'fields' => 'fr_rev_id,fr_timestamp,rev_timestamp,fr_quality,fr_user,user_name',
104	104	'conds' => $conds,
105		~~- 'options' => array( 'USE INDEX' => array( 'flaggedrevs' => 'PRIMARY' ) )~~
	105	+ //'options' => array( 'USE INDEX' => array( 'flaggedrevs' => 'page_rev' ) )
106	106	);
107	107	}
108	108
Index: branches/REL1_18/extensions/FlaggedRevs/presentation/FlaggedPageView.php
—	—	@@ -59,9 +59,9 @@
60	60	* or false if there isn't such a title
61	61	*/
62	62	public static function globalArticleInstance() {
63		~~- global $wgTitle;~~
64		~~- if ( !empty( $wgTitle ) ) {~~
65		~~- return FlaggedPage::getTitleInstance( $wgTitle );~~
	63	+ $title = RequestContext::getMain()->getTitle();
	64	+ if ( $title ) {
	65	+ return FlaggedPage::getTitleInstance( $title );
66	66	}
67	67	return null;
68	68	}
—	—	@@ -594,18 +594,21 @@
595	595	}
596	596	}
597	597
598		~~- # Check if this is a redirect...~~
599	598	$text = $frev->getRevText();
600		~~- $redirHtml = $this->getRedirectHtml( $text );~~
	599	+ # Get the new stable parser output...
	600	+ $pOpts = $this->article->makeParserOptions( $wgUser );
	601	+ $parserOut = FlaggedRevs::parseStableText(
	602	+ $this->article->getTitle(), $text, $frev->getRevId(), $pOpts );
601	603
602	604	# Parse and output HTML
603		~~- if ( $redirHtml == '' ) {~~
604		~~- $parserOptions = $this->article->makeParserOptions( $wgUser );~~
605		~~- $parserOut = FlaggedRevs::parseStableText(~~
606		~~- $this->article->getTitle(), $text, $frev->getRevId(), $parserOptions );~~
	605	+ $redirHtml = $this->getRedirectHtml( $text );
	606	+ if ( $redirHtml == '' ) { // page is not a redirect...
	607	+ # Add the stable output to the page view
607	608	$this->addParserOutput( $parserOut );
608		~~- } else {~~
	609	+ } else { // page is a redirect...
609	610	$this->out->addHtml( $redirHtml );
	611	+ # Add output to set categories, displaytitle, etc.
	612	+ $this->out->addParserOutputNoText( $parserOut );
610	613	}
611	614	}
612	615
—	—	@@ -673,30 +676,31 @@
674	677	}
675	678
676	679	# Get parsed stable version and output HTML
677		~~- $parserOptions = $this->article->makeParserOptions( $wgUser );~~
	680	+ $pOpts = $this->article->makeParserOptions( $wgUser );
678	681	$parserCache = FRParserCacheStable::singleton();
679		~~- $parserOut = $parserCache->get( $this->article, $parserOptions );~~
	682	+ $parserOut = $parserCache->get( $this->article, $pOpts );
680	683	if ( $parserOut ) {
	684	+ # Cache hit. Note that redirects are not cached.
681	685	$this->addParserOutput( $parserOut );
682	686	} else {
683	687	$text = $srev->getRevText();
684		~~- # Check if this is a redirect...~~
	688	+ # Get the new stable parser output...
	689	+ $parserOut = FlaggedRevs::parseStableText(
	690	+ $this->article->getTitle(), $text, $srev->getRevId(), $pOpts );
	691	+
685	692	$redirHtml = $this->getRedirectHtml( $text );
686		~~- # Don't parse redirects, use separate handling...~~
687		~~- if ( $redirHtml == '' ) {~~
688		~~- # Get the new stable output~~
689		~~- $parserOut = FlaggedRevs::parseStableText(~~
690		~~- $this->article->getTitle(), $text, $srev->getRevId(), $parserOptions );~~
	693	+ if ( $redirHtml == '' ) { // page is not a redirect...
691	694	# Update the stable version cache
692		~~- $parserCache->save( $parserOut, $this->article, $parserOptions );~~
	695	+ $parserCache->save( $parserOut, $this->article, $pOpts );
693	696	# Add the stable output to the page view
694	697	$this->addParserOutput( $parserOut );
695		-
696		~~- # Update the stable version dependancies~~
697		~~- FlaggedRevs::updateStableOnlyDeps( $this->article, $parserOut );~~
698		~~- } else {~~
	698	+ } else { // page is a redirect...
699	699	$this->out->addHtml( $redirHtml );
	700	+ # Add output to set categories, displaytitle, etc.
	701	+ $this->out->addParserOutputNoText( $parserOut );
700	702	}
	703	+ # Update the stable version dependancies
	704	+ FlaggedRevs::updateStableOnlyDeps( $this->article, $parserOut );
701	705	}
702	706
703	707	# Update page sync status for tracking purposes.
—	—	@@ -723,7 +727,8 @@
724	728	protected function getRedirectHtml( $text ) {
725	729	$rTargets = Title::newFromRedirectArray( $text );
726	730	if ( $rTargets ) {
727		~~- return $this->article->viewRedirect( $rTargets );~~
	731	+ $article = new Article( $this->article->getTitle() );
	732	+ return $article->viewRedirect( $rTargets );
728	733	}
729	734	return '';
730	735	}
—	—	@@ -904,7 +909,7 @@
905	910	* Adds stable version tags to page when editing
906	911	*/
907	912	public function addToEditView( EditPage $editPage ) {
908		~~- global $wgUser;~~
	913	+ global $wgUser, $wgParser;
909	914	$this->load();
910	915	# Must be reviewable. UI may be limited to unobtrusive patrolling system.
911	916	if ( !$this->article->isReviewable() ) {
—	—	@@ -956,7 +961,7 @@
957	962	$section = ( $editPage->section == "" ) ?
958	963	false : intval( $editPage->section );
959	964	if ( $section !== false ) {
960		~~- $text = $this->article->getSection( $text, $section );~~
	965	+ $text = $wgParser->getSection( $text, $section );
961	966	}
962	967	if ( $text !== false && strcmp( $text, $editPage->textbox1 ) !== 0 ) {
963	968	$diffEngine = new DifferenceEngine( $this->article->getTitle() );
—	—	@@ -1641,11 +1646,22 @@
1642	1647	array( ':' , '.' ), array( '%3A', '%' ), // hack: reverse encoding
1643	1648	substr( $sectionAnchor, 1 ) // remove the '#'
1644	1649	);
	1650	+<<<<<<< .working
1645	1651	$extraQuery .= '&fromsection=' . $section;
	1652	+=======
	1653	+ $params += array( 'fromsection' => $section );
	1654	+>>>>>>> .merge-right.r90388
1646	1655	$sectionAnchor = ''; // go to the top of the page to see notice
1647	1656	}
1648	1657	}
1649	1658	}
	1659	+<<<<<<< .working
	1660	+=======
	1661	+ if ( $extraQuery !== '' ) {
	1662	+ $extraQuery .= '&';
	1663	+ }
	1664	+ $extraQuery .= wfArrayToCGI( $params ); // note: EditPage will add initial "&"
	1665	+>>>>>>> .merge-right.r90388
1650	1666	return true;
1651	1667	}
1652	1668
Index: branches/REL1_18/extensions/FlaggedRevs/presentation/modules/flaggedrevs.css
—	—	@@ -23,10 +23,10 @@
24	24	background-color: #f0f8ff;
25	25	}
26	26	div.flaggedrevs_quality {
27		~~- background-color: #f0fff0;~~
	27	+ background-color: #e1ffe1;
28	28	}
29	29	div.flaggedrevs_pristine {
30		~~- background-color: #fffff0;~~
	30	+ background-color: #ffffe3;
31	31	}
32	32	div.flaggedrevs_notice {
33	33	background-color: #f9f9f9;
—	—	@@ -198,10 +198,10 @@
199	199	background-color: #f0f8ff;
200	200	}
201	201	.flaggedrevs-color-2 {
202		~~- background-color: #f0fff0;~~
	202	+ background-color: #e1ffe1;
203	203	}
204	204	.flaggedrevs-color-3 {
205		~~- background-color: #fffff0;~~
	205	+ background-color: #ffffe3;
206	206	}
207	207
208	208	.flaggedrevs-pending {
—	—	@@ -316,13 +316,13 @@
317	317	background-color: #f0f8ff;
318	318	}
319	319	.fr-rating-option-2 {
320		~~- background-color: #f0fff0;~~
	320	+ background-color: #e1ffe1;
321	321	}
322	322	.fr-rating-option-3 {
323	323	background-color: #fef0db;
324	324	}
325	325	.fr-rating-option-4 {
326		~~- background-color: #fffff0;~~
	326	+ background-color: #ffffe3;
327	327	}
328	328
329	329	.fr-diff-patrollink {
Property changes on: branches/REL1_18/extensions/FlaggedRevs
___________________________________________________________________
Modified: svn:mergeinfo
330	330	Merged /trunk/extensions/FlaggedRevs:r87606,87617,87669,87690,87700,90388,90419,90741,90789,90793,91363,91365,91584

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r87606	Follow-up r86179: Updated and commented out two USE INDEX statements. This *h...	aaron	23:44, 6 May 2011
r87617	Made "pristine" color a bit more visible	aaron	01:01, 7 May 2011
r87669	Run populate_fr_rev_timestamp() via updater	aaron	00:58, 8 May 2011
r87690	Added missing $dbw->timestamp() format call for new fr_rev_timestamp field	aaron	18:49, 8 May 2011
r87700	Tweak for bug 28598. Made insert() do strict insertions only and have the cal...	aaron	22:36, 8 May 2011
r90388	Fixed injectPostEditURLParams() query generation (& is already there)	aaron	08:50, 19 June 2011
r90419	Made "quality" color a bit more distinct	aaron	20:52, 19 June 2011
r90741	Fixed silly test bug	aaron	22:08, 24 June 2011
r90789	Fixed fatal due to removal of Article::getSection()	aaron	19:29, 25 June 2011
r90793	sigh...added bit left off of r90789	aaron	19:39, 25 June 2011
r91363	Fixed bogus reference to $status var (causes a msg exception later down)	aaron	18:32, 2 July 2011
r91365	Follow-up r91363: we need a msg key on failure	aaron	18:42, 2 July 2011
r91584	* Call viewRedirect() on Article (WikiPage doesn't inherit this anymore)...	aaron	19:13, 6 July 2011

Status & tagging log

00:20, 14 September 2011 Meno25 (talk | contribs) changed the status of r92317 [removed: old added: deferred]
19:21, 18 July 2011 Reedy (talk | contribs) changed the status of r92317 [removed: new added: old]