r77007 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r77006‎ \| r77007 \| r77008 >
Date:	06:16, 19 November 2010
Author:	aaron
Status:	deferred
Tags:
Comment:	Follow ups for r76962 and r75408: * Use rev IDs for both summary cases to avoid timestamp offset problems * Deal with 4+ authors case differently in default summary * Use review_bad_oldid error detection on pre-confirm reject rather than fatal * Reverted r76978, I shouldn't have suggested it (keys not reset) * Added sanity check to avoid insane queries (250 limit) * Fix for case when $this->comment is '0' * Shortened revreview-reject msg keys a bit * Added changetime field to RevisionReviewForm (for use later)
Modified paths:	/trunk/extensions/FlaggedRevs/forms/RevisionReviewForm.php (modified) (history) /trunk/extensions/FlaggedRevs/language/FlaggedRevs.i18n.php (modified) (history) /trunk/extensions/FlaggedRevs/specialpages/RevisionReview_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/FlaggedRevs/language/FlaggedRevs.i18n.php
—	—	@@ -256,8 +256,12 @@
257	257	'revreview-reject-summary' => 'Edit summary:',
258	258	'revreview-reject-confirm' => 'Reject these changes',
259	259	'revreview-reject-cancel' => 'Cancel',
260		~~- 'revreview-reject-default-summary-cur' => 'Rejected {{PLURAL:$1\|one change\|$1 changes}} by $2 to version as of $3',~~
261		~~- 'revreview-reject-default-summary-old' => 'Rejected {{PLURAL:$1\|one change\|$1 changes}} by $2',~~
	260	+ 'revreview-reject-summary-cur' => 'Rejected the last {{PLURAL:$1\|one change\|$1 changes}} (by $2) and restored revision $3',
	261	+ 'revreview-reject-summary-old' => 'Rejected the first {{PLURAL:$1\|one change\|$1 changes}} (by $2) following revision $3',
	262	+ 'revreview-reject-summary-cur-short' => 'Rejected the last {{PLURAL:$1\|one change\|$1 changes}} and restored revision $2',
	263	+ 'revreview-reject-summary-old-short' => 'Rejected the first {{PLURAL:$1\|one change\|$1 changes}} following revision $2',
	264	+ 'revreview-reject-usercount' => '{{PLURAL:$1\|one user\|$1 users}}',
	265	+ 'revreview-reject-toomanyedits' => 'Cannot reject this many edits at once.',
262	266
263	267	'revreview-reviewlink' => 'pending edits',
264	268	'revreview-reviewlink-title' => 'View diff of all pending changes',
—	—	@@ -577,12 +581,12 @@
578	582	'revreview-lev-quality' => '{{Flagged Revs}}',
579	583	'revreview-lev-pristine' => '{{Flagged Revs}}',
580	584	'revreview-reject-cancel' => '{{Identical\|Cancel}}',
581		~~- 'revreview-reject-default-summary-cur' => '{{Flagged Revs-small}}~~
	585	+ 'revreview-reject-summary-cur' => '{{Flagged Revs-small}}
582	586	Default summary shown when rejecting pending changes, and they are the latest revisions to a page
583	587	* $1 is the number of rejected revisions
584	588	* $2 is the list of (one or more) users who are being rejected
585	589	* $3 is the timestamp of the revision being reverted to',
586		~~- 'revreview-reject-default-summary-old' => '{{Flagged Revs-small}}~~
	590	+ 'revreview-reject-summary-old' => '{{Flagged Revs-small}}
587	591	Default summary shown when rejecting pending changes.
588	592	* $1 is the number of rejected revisions
589	593	* $2 is the list of (one or more) users who are being rejected',
Index: trunk/extensions/FlaggedRevs/forms/RevisionReviewForm.php
—	—	@@ -28,6 +28,7 @@
29	29	protected $notes = '';
30	30	protected $comment = '';
31	31	protected $dims = array();
	32	+ protected $lastChangeTime = '';
32	33
33	34	protected $oflags = array();
34	35	protected $inputLock = 0; # Disallow bad submissions
—	—	@@ -67,6 +68,10 @@
68	69	$this->trySet( $this->rejectConfirm, $value );
69	70	}
70	71
	72	+ public function setLastChangeTime( $value ) {
	73	+ $this->trySet( $this->lastChangeTime, $value );
	74	+ }
	75	+
71	76	public function getRefId() {
72	77	return $this->refid;
73	78	}
—	—	@@ -307,17 +312,17 @@
308	313	} elseif ( $this->getAction() === 'reject' ) {
309	314	$newRev = Revision::newFromTitle( $this->page, $this->oldid );
310	315	$oldRev = Revision::newFromTitle( $this->page, $this->refid );
311		-
312		~~- if( !$this->rejectConfirm ) {~~
313		~~- $this->rejectConfirmationForm( $oldRev, $newRev );~~
314		~~- return false;~~
315		~~- }~~
316	316	# Do not mess with archived/deleted revisions
317	317	if ( is_null( $oldRev ) \|\| $oldRev->mDeleted ) {
318	318	return 'review_bad_oldid';
319	319	} elseif ( is_null( $newRev ) \|\| $newRev->mDeleted ) {
320	320	return 'review_bad_oldid';
321	321	}
	322	+ # Go to confirmation screen first
	323	+ if ( !$this->rejectConfirm ) {
	324	+ $this->rejectConfirmationForm( $oldRev, $newRev );
	325	+ return false; // xxx
	326	+ }
322	327	$article = new Article( $this->page );
323	328	$new_text = $article->getUndoText( $newRev, $oldRev );
324	329	if ( $new_text === false ) {
—	—	@@ -634,6 +639,7 @@
635	640	$oldFlags = $frev
636	641	? $frev->getTags() // existing tags
637	642	: FlaggedRevs::quickTags( FR_CHECKED ); // basic tags
	643	+ $reviewTime = $frev ? $frev->getTimestamp() : ''; // last review of rev
638	644
639	645	# If we are reviewing updates to a page, start off with the stable revision's
640	646	# flags. Otherwise, we just fill them in with the selected revision's flags.
—	—	@@ -769,6 +775,7 @@
770	776	$form .= Html::hidden( 'oldid', $id ) . "\n";
771	777	$form .= Html::hidden( 'action', 'submit' ) . "\n";
772	778	$form .= Html::hidden( 'wpEditToken', $user->editToken() ) . "\n";
	779	+ $form .= Html::hidden( 'changetime', $reviewTime );
773	780	# Add review parameters
774	781	$form .= Html::hidden( 'templateParams', $templateParams ) . "\n";
775	782	$form .= Html::hidden( 'imageParams', $imageParams ) . "\n";
—	—	@@ -998,26 +1005,27 @@
999	1006	$wgOut->addHtml( '<div class="plainlinks">' );
1000	1007
1001	1008	$dbr = wfGetDB( DB_SLAVE );
1002		~~- $oldid = $dbr->addQuotes( $oldRev->getId() );~~
1003		~~- $newid = $dbr->addQuotes( $newRev->getId() );~~
1004	1009	$res = $dbr->select( 'revision',
1005	1010	array( 'rev_id', 'rev_user_text' ),
1006	1011	array(
1007		~~- 'rev_id > ' . $oldid,~~
1008		~~- 'rev_id <= ' . $newid,~~
1009		~~- 'rev_page' => $oldRev->getPage()~~
	1012	+ 'rev_page' => $oldRev->getPage(),
	1013	+ 'rev_id > ' . $dbr->addQuotes( $oldRev->getId() ),
	1014	+ 'rev_id <= ' . $dbr->addQuotes( $newRev->getId() )
1010	1015	),
1011		~~- __METHOD__~~
	1016	+ __METHOD__,
	1017	+ array( 'LIMIT' => 251 ) // sanity check
1012	1018	);
1013		~~- if ( !$dbr->numRows( $res ) ) { // sanity check~~
1014		~~- $wgOut->redirect( $this->getPage()->getFullUrl() );~~
	1019	+ if ( !$dbr->numRows( $res ) ) {
	1020	+ $wgOut->redirect( $this->getPage()->getFullUrl() ); // sanity check
1015	1021	return;
	1022	+ } elseif ( $dbr->numRows( $res ) > 250 ) {
	1023	+ $wgOut->showErrorPage( 'internalerror', 'revreview-reject-toomanyedits' );
	1024	+ return;
1016	1025	}
1017	1026
1018	1027	$rejectIds = array();
1019		~~- foreach( $res as $r ) {~~
1020		~~- $rejectIds[$r->rev_id] =~~
1021		~~- "[[User:{$r->rev_user_text}\|{$r->rev_user_text}]]";~~
	1028	+ foreach ( $res as $r ) {
	1029	+ $rejectIds[$r->rev_id] = "[[User:{$r->rev_user_text}\|{$r->rev_user_text}]]";
1022	1030	}
1023	1031
1024	1032	// List of revisions being undone...
—	—	@@ -1035,27 +1043,39 @@
1036	1044	}
1037	1045	}
1038	1046	$wgOut->addHtml( '</ul>' );
1039		~~- // Revision this will revert to (when reverting the top X revs)...~~
1040	1047	if ( $newRev->isCurrent() ) {
	1048	+ // Revision this will revert to (when reverting the top X revs)...
1041	1049	$wgOut->addWikiMsg( 'revreview-reject-text-revto',
1042	1050	$oldRev->getTitle()->getPrefixedDBKey(), $oldRev->getId(),
1043		~~- $wgLang->timeanddate( $oldRev->getTimestamp(), true ) );~~
1044		~~- $defaultSummary = wfMsgExt( 'revreview-reject-default-summary-cur',~~
1045		~~- array( 'parsemag' ),~~
1046		~~- $wgLang->formatNum( count( $rejectIds ) ),~~
1047		~~- $wgLang->listToText( array_unique( array_values( $rejectIds ) ) ),~~
1048	1051	$wgLang->timeanddate( $oldRev->getTimestamp(), true )
1049	1052	);
	1053	+ }
	1054	+
	1055	+ // Determine the default edit summary...
	1056	+ // NOTE: *-cur msg wording not safe for (unlikely) edit auto-merge
	1057	+ $rejectAuthors = array_values( array_unique( $rejectIds ) );
	1058	+ if ( count( $rejectAuthors ) > 3 ) {
	1059	+ $msg = $newRev->isCurrent()
	1060	+ ? 'revreview-reject-summary-cur-short'
	1061	+ : 'revreview-reject-summary-old-short';
	1062	+ $defaultSummary = wfMsgExt( $msg, 'parsemag',
	1063	+ $wgLang->formatNum( count( $rejectIds ) ), $oldRev->getId() );
1050	1064	} else {
1051		~~- $defaultSummary = wfMsgExt( 'revreview-reject-default-summary-old',~~
1052		~~- array( 'parsemag' ),~~
1053		~~- $wgLang->formatNum( count( $rejectIds ) ),~~
1054		~~- $wgLang->listToText( array_unique( array_values( $rejectIds ) ) )~~
	1065	+ $msg = $newRev->isCurrent()
	1066	+ ? 'revreview-reject-summary-cur'
	1067	+ : 'revreview-reject-summary-old';
	1068	+ $defaultSummary = wfMsgExt( $msg, 'parsemag',
	1069	+ $wgLang->formatNum( count( $rejectIds ) ),
	1070	+ $wgLang->listToText( $rejectAuthors ),
	1071	+ $oldRev->getId()
1055	1072	);
1056	1073	}
1057		-
1058		~~- if( $this->comment ) {~~
1059		~~- $defaultSummary = "{$defaultSummary}: {$this->comment}";~~
	1074	+ // Append any review comment...
	1075	+ if ( $this->comment != '' ) {
	1076	+ if ( $defaultSummary != '' ) {
	1077	+ $defaultSummary .= wfMsgForContent( 'colon-separator' );
	1078	+ }
	1079	+ $defaultSummary .= $this->comment;
1060	1080	}
1061	1081
1062	1082	$wgOut->addHtml( '</div>' );
—	—	@@ -1070,12 +1090,14 @@
1071	1091	$form .= Html::hidden( 'refid', $this->refid );
1072	1092	$form .= Html::hidden( 'target', $oldRev->getTitle()->getPrefixedDBKey() );
1073	1093	$form .= Html::hidden( 'wpEditToken', $this->user->editToken() );
	1094	+ $form .= Html::hidden( 'changetime', $newRev->getTimestamp() );
1074	1095	$form .= Xml::inputLabel( wfMsg( 'revreview-reject-summary' ), 'wpReason',
1075	1096	'wpReason', 120, $defaultSummary ) . "<br />";
1076	1097	$form .= Html::input( 'wpSubmit', wfMsg( 'revreview-reject-confirm' ), 'submit' );
1077	1098	$form .= Html::input( 'wpCancel', wfMsg( 'revreview-reject-cancel' ),
1078	1099	'button', array( 'onClick' => 'history.back();' ) );
1079	1100	$form .= Xml::closeElement( 'form' );
	1101	+
1080	1102	$wgOut->addHtml( $form );
1081	1103	}
1082	1104
Index: trunk/extensions/FlaggedRevs/specialpages/RevisionReview_body.php
—	—	@@ -56,6 +56,8 @@
57	57	$form->setFileVersion( $wgRequest->getVal( 'fileVersion' ) );
58	58	# Special token to discourage fiddling...
59	59	$form->setValidatedParams( $wgRequest->getVal( 'validatedParams' ) );
	60	+ # Conflict handling
	61	+ $form->setLastChangeTime( $wgRequest->getVal( 'changetime' ) );
60	62	# Tag values
61	63	foreach ( FlaggedRevs::getTags() as $tag ) {
62	64	# This can be NULL if we uncheck a checkbox

Follow-up revisions

Revision	Commit summary	Author	Date
r77008	Follow-up r77007: update qqq	aaron	06:28, 19 November 2010

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r75408	(bug 25294) "Reject" button confirmation screen in Pending Change	demon	02:36, 26 October 2010
r76962	Message cleanup for r75408:...	demon	21:25, 18 November 2010
r76978	Nitpick: switch array unique/values calls for clarity	demon	22:17, 18 November 2010

Status & tagging log

00:20, 14 September 2011 Meno25 (talk | contribs) changed the status of r77007 [removed: old added: deferred]
17:19, 23 December 2010 Reedy (talk | contribs) changed the status of r77007 [removed: new added: old]