r56326 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r56325‎ \| r56326 \| r56327 >
Date:	19:34, 14 September 2009
Author:	aaron
Status:	ok (Comments)
Tags:
Comment:	* Rate-limit review * Improve failure messages * Use status codes/other minor cleanups
Modified paths:	/trunk/extensions/ReaderFeedback/ReaderFeedback.php (modified) (history) /trunk/extensions/ReaderFeedback/language/ReaderFeedback.i18n.php (modified) (history) /trunk/extensions/ReaderFeedback/specialpages/ReaderFeedback_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ReaderFeedback/specialpages/ReaderFeedback_body.php
—	—	@@ -6,6 +6,10 @@
7	7
8	8	class ReaderFeedbackPage extends UnlistedSpecialPage
9	9	{
	10	+ const REVIEW_ERROR = 0;
	11	+ const REVIEW_OK = 1;
	12	+ const REVIEW_DUP = 2;
	13	+
10	14	// Initialize to handle incomplete AJAX input
11	15	var $page = null;
12	16	var $oldid = 0;
—	—	@@ -14,7 +18,7 @@
15	19	var $commentary = '';
16	20
17	21	public function __construct() {
18		~~- UnlistedSpecialPage::UnlistedSpecialPage( 'ReaderFeedback', 'feedback' );~~
	22	+ parent::__construct( 'ReaderFeedback', 'feedback' );
19	23	wfLoadExtensionMessages( 'ReaderFeedback' );
20	24	}
21	25
—	—	@@ -23,16 +27,13 @@
24	28	$confirm = $wgRequest->wasPosted() && $wgUser->matchEditToken( $wgRequest->getVal( 'wpEditToken' ) );
25	29	if( $wgUser->isAllowed( 'feedback' ) ) {
26	30	if( $wgUser->isBlocked( !$confirm ) ) {
27		~~- $wgOut->blockedPage();~~
28		~~- return;~~
	31	+ return $wgOut->blockedPage();
29	32	}
30	33	} else {
31		~~- $wgOut->permissionRequired( 'feedback' );~~
32		~~- return;~~
	34	+ return $wgOut->permissionRequired( 'feedback' );
33	35	}
34	36	if( wfReadOnly() ) {
35		~~- $wgOut->readOnlyPage();~~
36		~~- return;~~
	37	+ return $wgOut->readOnlyPage();
37	38	}
38	39	$this->setHeaders();
39	40	# Our target page
—	—	@@ -73,11 +74,11 @@
74	75	if( $confirm && !$wgRequest->getVal( 'commentary' ) ) {
75	76	$ok = $this->submit();
76	77	} else {
77		~~- $ok = false;~~
	78	+ $ok = self::REVIEW_ERROR;
78	79	}
79	80	# Go to graphs!
80	81	global $wgMiserMode;
81		~~- if( $ok && !$wgMiserMode ) {~~
	82	+ if( $ok == self::REVIEW_OK && !$wgMiserMode ) {
82	83	$ratingTitle = SpecialPage::getTitleFor( 'RatingHistory' );
83	84	$wgOut->redirect( $ratingTitle->getLocalUrl('target='.$this->page->getPrefixedUrl() ) );
84	85	# Already voted or graph is set to be skipped...
—	—	@@ -169,16 +170,24 @@
170	171
171	172	$dbw = wfGetDB( DB_MASTER );
172	173	$dbw->begin();
173		~~- $ok = ( $bot \|\| $form->submit() ); // don't submit for mindless drones~~
174		~~- $dbw->commit();~~
175		~~- if( $ok ) {~~
176		~~- return '<suc#>'.wfMsgExt( 'readerfeedback-success', array('parseinline'),~~
177		~~- $form->page->getPrefixedText(), $graphLink, $talk->getFullUrl( 'action=edit&section=new' ) ) .~~
178		~~- '<h4>'.wfMsgHtml('ratinghistory-table')."</h4>\n$tallyTable";~~
	174	+ if( $bot ) {
	175	+ $ok = self::REVIEW_ERROR; // don't submit for mindless drones
179	176	} else {
180		~~- return '<err#>'.wfMsgExt( 'readerfeedback-voted', array('parseinline'),~~
181		~~- $form->page->getPrefixedText(), $graphLink, $talk->getFullUrl( 'action=edit&section=new' ) );~~
	177	+ $ok = $form->submit();
182	178	}
	179	+ $dbw->commit();
	180	+ switch( $ok ) {
	181	+ case self::REVIEW_OK:
	182	+ return '<suc#>'.wfMsgExt( 'readerfeedback-success', array('parseinline'),
	183	+ $form->page->getPrefixedText(), $graphLink, $talk->getFullUrl( 'action=edit&section=new' ) ) .
	184	+ '<h4>'.wfMsgHtml('ratinghistory-table')."</h4>\n$tallyTable";
	185	+ case self::REVIEW_DUP:
	186	+ return '<err#>'.wfMsgExt( 'readerfeedback-voted', array('parseinline'),
	187	+ $form->page->getPrefixedText(), $graphLink, $talk->getFullUrl( 'action=edit&section=new' ) );
	188	+ default:
	189	+ return '<err#>'.wfMsgExt( 'readerfeedback-error', array('parseinline'),
	190	+ $form->page->getPrefixedText(), $graphLink, $talk->getFullUrl( 'action=edit&section=new' ) );
	191	+ }
183	192	}
184	193
185	194	protected static function isValid( $int ) {
—	—	@@ -256,21 +265,25 @@
257	266	$now = wfTimestampNow();
258	267	$date = str_pad( substr( $now, 0, 8 ), 14, '0' );
259	268	if( count($this->dims) == 0 )
260		~~- return false;~~
	269	+ return self::REVIEW_ERROR;
261	270	$ratings = $this->flattenRatings( $this->dims );
262	271	# Make sure revision is valid!
263	272	$rev = Revision::newFromId( $this->oldid );
264	273	if( !$rev \|\| !$rev->getTitle()->equals( $this->page ) ) {
265		~~- return false; // opps!~~
	274	+ return self::REVIEW_ERROR; // opps!
266	275	}
267	276	$ip = wfGetIP();
268	277	if( !$wgUser->getId() && !$ip ) {
269		~~- return false; // we need to keep track somehow~~
	278	+ return self::REVIEW_ERROR; // we need to keep track somehow
270	279	}
271	280	$article = new Article( $this->page );
	281	+ # Check if the user is spamming reviews...
	282	+ if( $wgUser->pingLimiter( 'feedback' ) \|\| $wgUser->pingLimiter() ) {
	283	+ return self::REVIEW_ERROR;
	284	+ }
272	285	# Check if user already voted before...
273	286	if( self::userAlreadyVoted( $this->page, $this->oldid ) ) {
274		~~- return false;~~
	287	+ return self::REVIEW_DUP;
275	288	}
276	289	# Update review records to limit double voting!
277	290	$insertRow = array(
—	—	@@ -328,6 +341,6 @@
329	342	if( $wgUser->getId() ) {
330	343	$this->page->invalidateCache();
331	344	}
332		~~- return true;~~
	345	+ return self::REVIEW_OK;
333	346	}
334	347	}
Index: trunk/extensions/ReaderFeedback/language/ReaderFeedback.i18n.php
—	—	@@ -34,6 +34,7 @@
35	35	'readerfeedback-main' => 'Only content pages can be rated.',
36	36	'readerfeedback-success' => '\'\'\'Thank you for reviewing this page!\'\'\' ([$3 Comments or questions?]).',
37	37	'readerfeedback-voted' => '\'\'\'It appears that you already rated this page\'\'\' ([$3 Comments or questions?]).',
	38	+ 'readerfeedback-error' => '\'\'\'An error has occurred while rating this page\'\'\' ([$3 Comments or questions?]).',
38	39	'readerfeedback-submitting' => 'Submitting …',
39	40	'readerfeedback-finished' => 'Thank you!',
40	41	'readerfeedback-tagfilter' => 'Tag:',
Index: trunk/extensions/ReaderFeedback/ReaderFeedback.php
—	—	@@ -58,6 +58,14 @@
59	59	$wgFeedbackAge = 7 * 24 * 3600;
60	60	# How long before stats page is updated?
61	61	$wgFeedbackStatsAge = 2 * 3600; // 2 hours
	62	+# Limit people from spamming the system
	63	+# (uses count => seconds tuples)
	64	+$wgRateLimits['feedback'] = array(
	65	+ 'newbie' => array( 5, 60 ), // for each recent (autoconfirmed) account; overrides 'user'
	66	+ 'user' => null, // for each logged-in user
	67	+ 'ip' => array( 5, 60 ), // for each anon and recent account
	68	+ 'subnet' => null, // ... with final octet removed
	69	+);
62	70
63	71	# URL location for readerfeedback.css and readerfeedback.js
64	72	# Use a literal $wgScriptPath as a placeholder for the runtime value of $wgScriptPath

Follow-up revisions

Revision	Commit summary	Author	Date
r59620	Reverted return style changes from r56326 (just used to reduce code lines)	aaron	08:03, 1 December 2009

Comments

#Comment by Tim Starling (talk | contribs) 05:58, 1 December 2009

-			$wgOut->readOnlyPage();
-			return;
+			return $wgOut->readOnlyPage();

The first version was correct. Don't pretend to use the return value of a function that doesn't return anything, it's confusing.

Status & tagging log

01:41, 9 December 2009 Tim Starling (talk | contribs) changed the status of r56326 [removed: fixme added: ok]
05:58, 1 December 2009 Tim Starling (talk | contribs) changed the status of r56326 [removed: new added: fixme]