r109582 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r109581‎ \| r109582 \| r109583 >
Date:	23:16, 19 January 2012
Author:	gregchiasson
Status:	resolved (Comments)
Tags:
Comment:	AFT5 - Add permissions/roll checks to AFT5 feedback page and API calls. Requirements call for additional permissions on features that don't exist yet, which is why there are more roles than we seem to need.
Modified paths:	/trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/SpecialArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5Utils.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php
—	—	@@ -32,6 +32,7 @@
33	33	// error messages
34	34	'articlefeedbackv5-error' => 'An error has occured. Please try again later.',
35	35	'articlefeedbackv5-error-email' => 'That e-mail address is not valid.',
	36	+ 'articlefeedbackv5-error-blocked' => 'Blocked users may not submit feedback.',
36	37	'articlefeedbackv5-error-validation' => 'Validation error.',
37	38	'articlefeedbackv5-error-abuse' => 'Your comment violates the $1. Please revise it.',
38	39	'articlefeedbackv5-error-abuse-linktext' => 'feedback abuse policy',
Index: trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5.php
—	—	@@ -33,6 +33,13 @@
34	34	global $wgUser, $wgArticleFeedbackv5SMaxage;
35	35	$params = $this->extractRequestParams();
36	36
	37	+ // Blocked users are, well, blocked.
	38	+ if( $wgUser->isBlocked() ) {
	39	+ $this->getResult()->addValue( null, 'error', 'articlefeedbackv5-error-blocked' );
	40	+ return;
	41	+ }
	42	+
	43	+
37	44	// Anon token check
38	45	$token = $this->getAnonToken( $params );
39	46
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
—	—	@@ -14,12 +14,13 @@
15	15	* @subpackage Api
16	16	*/
17	17	class ApiViewFeedbackArticleFeedbackv5 extends ApiQueryBase {
18		-
	18	+ private $access = array();
19	19	/**
20	20	* Constructor
21	21	*/
22	22	public function __construct( $query, $moduleName ) {
23	23	parent::__construct( $query, $moduleName, 'afvf' );
	24	+ $this->access = ApiArticleFeedbackv5Utils::initializeAccess();
24	25	}
25	26
26	27	/**
—	—	@@ -185,6 +186,16 @@
186	187
187	188	private function getFilterCriteria( $filter, $filterValue = null ) {
188	189	$where = array();
	190	+
	191	+ // Permissions check
	192	+ if(
	193	+ ( $filter == 'invisible' && !$this->access[ 'rollbackers' ] )
	194	+ \|\| ( $filter == 'deleted' && !$this->access[ 'oversight' ] )
	195	+
	196	+ ) {
	197	+ $filter = null;
	198	+ }
	199	+
189	200	switch( $filter ) {
190	201	case 'all':
191	202	$where = array();
—	—	@@ -217,10 +228,10 @@
218	229	default: $content .= $this->renderNoBucket( $record ); break;
219	230	}
220	231	# TODO: check roles to determine what to show here (and cache somewhere so we don't keep looking them up).
221		~~- $can_flag = 1;~~
222		~~- $can_upvote = 1;~~
223		~~- $can_hide = 1;~~
224		~~- $can_delete = 1;~~
	232	+ $can_flag = !$this->access[ 'blocked' ];
	233	+ $can_vote = !$this->access[ 'blocked' ];
	234	+ $can_hide = $this->access[ 'rollbackers' ];
	235	+ $can_delete = $this->access[ 'oversight' ];
225	236	$id = $record[0]->af_id;
226	237
227	238	# $header_links = Html::openElement( 'p', array( 'class' => 'articleFeedbackv5-comment-head' ) )
—	—	@@ -251,7 +262,7 @@
252	263
253	264	$footer_links = Html::openElement( 'p', array( 'class' => 'articleFeedbackv5-comment-foot' ) );
254	265
255		~~- if( $can_upvote ) {~~
	266	+ if( $can_vote ) {
256	267	$footer_links .= Html::element( 'span', array(
257	268	'class' => 'articleFeedbackv5-helpful-caption'
258	269	), wfMessage( 'articlefeedbackv5-form-helpful-label', ( $record[0]->af_helpful_count + $record[0]->af_unhelpful_count ) ) )
—	—	@@ -286,6 +297,7 @@
287	298	. Html::openElement( 'ul', array(
288	299	'id' => 'articleFeedbackv5-feedback-tools-list-'.$id
289	300	) )
	301	+ # TODO: unhide hidden posts
290	302	. ( $can_hide ? Html::rawElement( 'li', array(), Html::element( 'a', array(
291	303	'id' => "articleFeedbackv5-hide-link-$id",
292	304	'class' => 'articleFeedbackv5-hide-link'
—	—	@@ -294,6 +306,8 @@
295	307	'id' => "articleFeedbackv5-abuse-link-$id",
296	308	'class' => 'articleFeedbackv5-abuse-link'
297	309	), wfMessage( 'articlefeedbackv5-form-abuse', $record[0]->af_abuse_count )->text() ) ) : '' )
	310	+ # TODO: nonoversight can mark for oversight, oversight can
	311	+ # either delete or un-delete, based on deletion status
298	312	. ( $can_delete ? Html::rawElement( 'li', array(), Html::element( 'a', array(
299	313	'id' => "articleFeedbackv5-delete-link-$id",
300	314	'class' => 'articleFeedbackv5-delete-link'
—	—	@@ -490,5 +504,4 @@
491	505	public function getVersion() {
492	506	return __CLASS__ . ': $Id: ApiViewRatingsArticleFeedbackv5.php 103439 2011-11-17 03:19:01Z rsterbin $';
493	507	}
494		-
495	508	}
Index: trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5Utils.php
—	—	@@ -190,5 +190,18 @@
191	191
192	192	$dbw->commit();
193	193	}
	194	+
	195	+ public function initializeAccess() {
	196	+ global $wgUser;
	197	+ return array(
	198	+ 'blocked' => $wgUser->isBlocked(),
	199	+ 'anon' => $wgUser->isAnon(),
	200	+ 'registered' => !$wgUser->isAnon() && !$wgUser->isBlocked(),
	201	+ 'autoconfirmed' => in_array('autoconfirmed', $wgUser->getEffectiveGroups()),
	202	+ 'rollbackers' => in_array('rollbacker', $wgUser->getEffectiveGroups()),
	203	+ 'admins' => in_array('sysop', $wgUser->getEffectiveGroups()),
	204	+ 'oversight' => in_array('oversight', $wgUser->getEffectiveGroups())
	205	+ );
	206	+ }
194	207	}
195	208
Index: trunk/extensions/ArticleFeedbackv5/SpecialArticleFeedbackv5.php
—	—	@@ -15,18 +15,31 @@
16	16	* @subpackage Special
17	17	*/
18	18	class SpecialArticleFeedbackv5 extends SpecialPage {
	19	+ private $access;
19	20	private $filters = array(
20	21	'visible',
21		~~- 'invisible',~~
22	22	'all',
23	23	'comment'
24	24	);
	25	+ private $sorts = array(
	26	+ 'newest',
	27	+ 'oldest',
	28	+ 'helpful'
	29	+ );
25	30
26	31	/**
27	32	* Constructor
28	33	*/
29	34	public function __construct() {
30	35	parent::__construct( 'ArticleFeedbackv5' );
	36	+ $this->access = ApiArticleFeedbackv5Utils::initializeAccess();
	37	+
	38	+ if( $this->access[ 'rollbackers' ] ) {
	39	+ $filter[] = 'invisible';
	40	+ }
	41	+ if( $this->access[ 'oversight' ] ) {
	42	+ $filter[] = 'deleted';
	43	+ }
31	44	}
32	45
33	46	/**
—	—	@@ -121,8 +134,7 @@
122	135	$out->addModules( 'jquery.articleFeedbackv5.special' );
123	136
124	137	$sortLabels = array();
125		~~- $sortOpts = array( 'newest', 'oldest', 'helpful' );~~
126		~~- foreach ( $sortOpts as $sort ) {~~
	138	+ foreach ( $this->sorts as $sort ) {
127	139	$sortLabels[] = Html::element(
128	140	'a',
129	141	array(

Follow-up revisions

Revision	Commit summary	Author	Date
r109624	AFT5 - followup to r109582, replace spaces with tabs.	gregchiasson	16:09, 20 January 2012

Comments

#Comment by Santhosh.thottingal (talk | contribs) 09:27, 20 January 2012

please add message documentation for articlefeedbackv5-error-blocked in $messages['qqq']

#Comment by Johnduhart (talk | contribs) 14:41, 20 January 2012

Mixes spaces and tabs.

#Comment by Gregchiasson (talk | contribs) 22:27, 24 January 2012

Spaces and tabs were fixed in r109624, and the articlefeedbackv5-error-blocked documentation was added in r109620 (which, admittedly, I forgot to tag as a followup to this).

#Comment by Catrope (talk | contribs) 05:59, 31 January 2012

+                        'autoconfirmed' => in_array('autoconfirmed', $wgUser->getEffectiveGroups()),
+                        'rollbackers'   => in_array('rollbacker', $wgUser->getEffectiveGroups()),
+                        'admins'        => in_array('sysop', $wgUser->getEffectiveGroups()),
+                        'oversight'     => in_array('oversight', $wgUser->getEffectiveGroups())

Please don't hardcode group names like this, use rights instead, like this:

// Check if the user has a given right
if ( $wgUser->isAllowed( 'articlefeedbackv5-viewdeletedfeedback' ) ) { ... }

// Add this to ArticleFeedbackv5.php to add the right
$wgAvailableRights[] = 'articlefeedbackv5-viewdeletedfeedback';

// Add an i18n message describing the right in ArticleFeedbackv5.i18n.php
'right-articlefeedbackv5-viewdeletedfeedback' => 'View deleted ArticleFeedback comments',

// To assign the right to a group, put this in LocalSettings.php:
$wgGroupPermissions['sysop']['articlefeedbackv5-viewdeletedfeedback'] = true;

OK otherwise.

#Comment by Catrope (talk | contribs) 06:56, 31 January 2012

Groups/rights stuff fixed in r109931.

Status & tagging log

01:28, 13 June 2012 Krinkle (talk | contribs) changed the tags for r109582 [removed: nodeploy]
06:56, 31 January 2012 Catrope (talk | contribs) changed the status of r109582 [removed: fixme added: resolved]
05:59, 31 January 2012 Catrope (talk | contribs) changed the status of r109582 [removed: new added: fixme]
23:17, 27 January 2012 Gregchiasson (talk | contribs) changed the status of r109582 [removed: fixme added: new]
23:51, 26 January 2012 MarkAHershberger (talk | contribs) changed the tags for r109582 [added: nodeploy]
14:41, 20 January 2012 Johnduhart (talk | contribs) changed the status of r109582 [removed: new added: fixme]