r63722 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r63721‎ \| r63722 \| r63723 >
Date:	01:36, 14 March 2010
Author:	jeroendedauw
Status:	resolved (Comments)
Tags:
Comment:	* Added (untested) stuff to ApiStoryReview to enable ajax actions on Storyreview page. * Added query to get stories on Special:Story + new db index * Minor layout change to storyboard tag
Modified paths:	/trunk/extensions/Storyboard/Storyboard.i18n.php (modified) (history) /trunk/extensions/Storyboard/api/ApiStoryReview.php (modified) (history) /trunk/extensions/Storyboard/specials/Story/Story_body.php (modified) (history) /trunk/extensions/Storyboard/storyboard.sql (modified) (history) /trunk/extensions/Storyboard/tags/Storyboard/storyboard.css (modified) (history) /trunk/extensions/Storyboard/tags/Storyboard/storyboard.js (modified) (history)

Diff [purge]

Index: trunk/extensions/Storyboard/specials/Story/Story_body.php
—	—	@@ -19,12 +19,61 @@
20	20	parent::__construct( 'Story' );
21	21	}
22	22
23		~~- public function execute( $language ) {~~
	23	+ public function execute( $identifier ) {
24	24	wfProfileIn( __METHOD__ );
25	25
26		~~- global $wgOut;~~
27		~~- $wgOut->addHTML( '' ); // TODO: add output~~
	26	+ if ( trim($identifier) == '' ) {
	27	+ global $wgOut;
	28	+ $wgOut->addHTML( wfMsg( 'storyboard-nostorytitle' ) );
	29	+ return;
	30	+ }
28	31
	32	+ $dbr = wfGetDB( DB_SLAVE );
	33	+
	34	+ if ( is_numeric( $identifier ) ) {
	35	+ $conds = array(
	36	+ 'story_id' => $identifier
	37	+ );
	38	+ } else {
	39	+ $conds = array(
	40	+ 'story_title' => str_replace( '_', ' ', $identifier ) // TODO: escaping required?
	41	+ );
	42	+ }
	43	+
	44	+ $stories = $dbr->Select(
	45	+ 'storyboard',
	46	+ array(
	47	+ 'story_id',
	48	+ 'story_author_name',
	49	+ 'story_title',
	50	+ 'story_text',
	51	+ 'story_created',
	52	+ 'story_is_published',
	53	+ ),
	54	+ $conds
	55	+ );
	56	+
	57	+ $story = $dbr->fetchObject( $stories );
	58	+
	59	+ if ( $story ) {
	60	+ if ( $story->story_is_published == 1 ) {
	61	+ $this->showStory( $story );
	62	+ }
	63	+ else {
	64	+ $wgOut->addHTML( wfMsg( 'storyboard-unpublished' ) );
	65	+ }
	66	+ }
	67	+ else {
	68	+ global $wgOut;
	69	+ $wgOut->addHTML( wfMsg( 'storyboard-nosuchstory' ) );
	70	+ }
	71	+
29	72	wfProfileOut( __METHOD__ );
30	73	}
	74	+
	75	+ private function showStory( $story ) {
	76	+ global $wgOut;
	77	+
	78	+ $wgOut->addHTML( '' ); // TODO: add output
	79	+ }
31	80	}
\ No newline at end of file
Index: trunk/extensions/Storyboard/storyboard.sql
—	—	@@ -17,3 +17,4 @@
18	18	) /$wgDBTableOptions/;
19	19
20	20	CREATE INDEX story_published_modified ON /$wgDBprefix/storyboard (story_is_published, story_modified);
	21	+CREATE INDEX story_title ON /$wgDBprefix/storyboard (story_title);
\ No newline at end of file
Index: trunk/extensions/Storyboard/api/ApiStoryReview.php
—	—	@@ -43,33 +43,85 @@
44	44	global $wgUser;
45	45
46	46	if ( !$wgUser->isAllowed( 'storyreview' ) \|\| $wgUser->isBlocked() ) {
47		~~- $this->dieUsageMsg( reset( $retval ) );~~
48		~~- $this->dieUsageMsg( array( 'notanarticle' ) );~~
	47	+ $this->dieUsageMsg( array( 'storyreview' ) );
49	48	}
50	49
51		~~- // TODO~~
	50	+ $params = $this->extractRequestParams();
52	51
53		~~- }~~
54		-
55		~~- private static function getPermissionsError( &$title, $token ) {~~
56		~~- global $wgUser;~~
	52	+ // Check required parameters
	53	+ if ( !array_key_exists( 'storyid', $params ) ) {
	54	+ $this->dieUsageMsg( array( 'missingparam', 'storyid' ) );
	55	+ }
	56	+ if ( !array_key_exists( 'storyaction', $params ) ) {
	57	+ $this->dieUsageMsg( array( 'missingparam', 'storyaction' ) );
	58	+ }
	59	+
	60	+ // TODO: test the actions after using them in the storyreview special page
	61	+ $dbw = wfGetDB( DB_MASTER );
57	62
58		~~- // Check permissions~~
59		~~- $errors = $title->getUserPermissionsErrors( 'storyreview', $wgUser );~~
60		~~- if ( count( $errors ) > 0 ) {~~
61		~~- return $errors;~~
	63	+ if ( $params['storyaction'] == 'delete' ) {
	64	+ // TODO: does this need to be escaped, or is putting the type of the param to integer sufficient?
	65	+ $dbw->delete( 'storyboard', "story_id = '$params[storyid]'" );
	66	+ } else {
	67	+ $conds = array(
	68	+ 'story_id' => $params['storyid']
	69	+ );
	70	+
	71	+ switch( $params['storyaction'] ) {
	72	+ case 'hide' :
	73	+ $values = array(
	74	+ 'story_is_hidden' => 1
	75	+ );
	76	+ break;
	77	+ case 'unhide' :
	78	+ $values = array(
	79	+ 'story_is_hidden' => 0
	80	+ );
	81	+ break;
	82	+ case 'publish' :
	83	+ $values = array(
	84	+ 'story_is_published' => 1
	85	+ );
	86	+ break;
	87	+ case 'unpublish' :
	88	+ $values = array(
	89	+ 'story_is_published' => 0
	90	+ );
	91	+ break;
	92	+ case 'hideimage' :
	93	+ $values = array(
	94	+ 'story_image_hidden' => 1
	95	+ );
	96	+ break;
	97	+ case 'showimage' :
	98	+ $values = array(
	99	+ 'story_image_hidden' => 0
	100	+ );
	101	+ break;
	102	+ case 'deleteimage' :
	103	+ $values = array(
	104	+ 'story_author_image' => ''
	105	+ );
	106	+ break;
	107	+ }
	108	+
	109	+ $dbw->update( 'storyboard', $values, $conds );
62	110	}
63		-
64		~~- return array();~~
65		~~- }~~
	111	+ }
66	112
67	113	public function getAllowedParams() {
68	114	return array(
	115	+ 'storyid' => array(
	116	+ ApiBase :: PARAM_TYPE => 'integer',
	117	+ ),
	118	+ 'storyaction' => null,
69	119	);
70	120	}
71	121
72	122	public function getParamDescription() {
73	123	return array(
	124	+ 'storyid' => '',
	125	+ 'storyaction' => '',
74	126	);
75	127	}
76	128
—	—	@@ -81,12 +133,16 @@
82	134
83	135	public function getPossibleErrors() {
84	136	return array_merge( parent::getPossibleErrors(), array(
	137	+ array( 'missingparam', 'storyid' ),
	138	+ array( 'missingparam', 'storyaction' ),
85	139	) );
86	140	}
87	141
88	142	protected function getExamples() {
89	143	return array(
90		~~- 'api.php?action=storyreview&...' // TODO~~
	144	+ 'api.php?action=storyreview&storyid=42&storyaction=publish',
	145	+ 'api.php?action=storyreview&storyid=42&storyaction=hide',
	146	+ 'api.php?action=storyreview&storyid=42&storyaction=delete',
91	147	);
92	148	}
93	149
Index: trunk/extensions/Storyboard/Storyboard.i18n.php
—	—	@@ -20,6 +20,11 @@
21	21
22	22	'right-storyreview' => 'Review, edit, publish, and hide stories',
23	23
	24	+ // Special:Story
	25	+ 'storyboard-nosuchstory' => 'The story you requested does not exist. It might have been removed.',
	26	+ 'storyboard-unpublished' => 'The story you requested has not been published yet.',
	27	+ 'storyboard-nostorytitle' => 'You need to specify the title or id of the story you want to view.',
	28	+
24	29	// Story review
25	30	'storyboard-storyreview' => 'Story review',
26	31	'storyboard-publish' => 'Publish',
Index: trunk/extensions/Storyboard/tags/Storyboard/storyboard.css
—	—	@@ -51,5 +51,5 @@
52	52	}
53	53	.storyboard-image {
54	54	margin: 5px 15px 0px 15px;
55		~~- float: left;~~
	55	+ float: right;
56	56	}
\ No newline at end of file
Index: trunk/extensions/Storyboard/tags/Storyboard/storyboard.js
—	—	@@ -29,12 +29,6 @@
30	30	var story = data.query.stories[i];
31	31	var $storyBody = $( "<div />" ).addClass( "storyboard-box" );
32	32
33		~~- $( "<img />" )~~
34		~~- // TODO: replace by wgScriptPath + path/to/scropped/img~~
35		~~- .attr( "src", "http://upload.wikimedia.org/wikipedia/mediawiki/9/99/SemanticMaps.png" )~~
36		~~- .addClass( "storyboard-image" )~~
37		~~- .appendTo( $storyBody );~~
38		-
39	33	var $header = $( "<div />" ).addClass( "storyboard-header" ).appendTo( $storyBody );
40	34	$( "<div />" ).addClass( "storyboard-title" ).text( story.title ).appendTo( $header );
41	35
—	—	@@ -75,7 +69,15 @@
76	70	) //TODO
77	71	.appendTo( $header );
78	72
79		- $storyBody.append( $( "<div />" ).text( story.text ).addClass( "storyboard-text" ) ); // Lorem ipsum dolor sit amet, consectetur adipiscing elit. Mauris dapibus, nibh vitae blandit tincidunt, risus libero vehicula mauris, nec interdum mi lectus lobortis turpis. Suspendisse vel magna dapibus purus iaculis hendrerit nec ut lectus. Nullam in turpis sed elit volutpat accumsan id quis lectus. Phasellus a felis lectus. Donec erat neque, tincidunt sit amet tempus non, malesuada vel justo. Vestibulum eget magna enim, quis malesuada lorem. Integer rutrum scelerisque adipiscing. Proin feugiat tincidunt ultrices. Vivamus at justo turpis, ut porta mi. Fusce nisl eros, luctus non accumsan eget, varius id urna. Quisque hendrerit, neque eu varius sollicitudin, lacus nisl auctor odio, eget egestas turpis sapien ut diam. Quisque ultricies consequat erat in tempor. Nam ut libero ac massa volutpat vestibulum vel sed leo. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Aenean pharetra, nisi ut dignissim semper, nunc lectus elementum dolor, sit amet blandit mauris diam ut nisi. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Etiam fringilla ultricies dapibus. Donec tristique congue lorem eget varius. Nunc nunc orci, molestie et elementum eget, pulvinar ac ante. Nam fermentum est ut lorem luctus suscipit consectetur ligula gravida. Phasellus non magna sit amet lectus feugiat malesuada eu non lorem. Nam commodo fermentum mauris, sed vehicula risus molestie et. Sed nisl neque, mollis sit amet malesuada ac, bibendum vitae mauris. Quisque dictum viverra eros quis gravida. Etiam vitae augue risus. Donec at orci vitae mauris luctus porttitor.
	73	+ $storyBody.append( $( "<div />" ).addClass( "storyboard-text" )
	74	+ .text( story["*"] )
	75	+ .prepend( $( "<img />" )
	76	+ // TODO: replace by wgScriptPath + path/to/scropped/img
	77	+ .attr( "src", "http://upload.wikimedia.org/wikipedia/mediawiki/9/99/SemanticMaps.png" )
	78	+ .addClass( "storyboard-image" )
	79	+ )
	80	+ );
	81	+
80	82	$div.append( $storyBody );
81	83	}
82	84	$storyboard.html( $div );

Follow-up revisions

Revision	Commit summary	Author	Date
r63758	Partial follow up to r63722 - will test the API module later today	jeroendedauw	20:48, 14 March 2010

Comments

#Comment by Nikerabbit (talk | contribs) 08:31, 14 March 2010

+'story_title' => str_replace( '_', ' ', $identifier ) // TODO: escaping required?

In this case no. The database wrappers escape almost everything for you.

+if ( is_numeric( $identifier ) ) {

is_numeric accepts all kinds of weird stuff, is that what you want?

+$wgOut->addHTML( wfMsg( 'storyboard-nostorytitle' ) );

Please use $wgOut->addWikiMsg( 'name ); if it doesn't hurt to parse the messages, or $wgOut->addHTML( wfMsgExt( 'name', array( 'parsemag', 'escape' ) ) );, if you only want to do {{-transformation and escape the output.

+$dbw->delete( 'storyboard', "story_id = '$params[storyid]'" );

Even though the api may validate the type for you, I would escape this. Does array( 'story_id' => $params['storyid'] ) work here? Quotes were also missing around the array key, which should produce PHP error notices.

+$stories = $dbr->Select(

Nitpick, select in lower case :) Would it be possible to use selectRow() here?

#Comment by Catrope (talk | contribs) 13:04, 14 March 2010

Yes, is_numeric accepts weird crap like "0x0ef3", not to mention the fact that there's no reason why story titles couldn't be numeric. If someone creates a story called "123" and it doesn't happen to also have ID 123, it won't be reachable in the usual way. For this reason, I recommend you move the ID to a normal GET parameter, so you'd have URLs like http://en.wikipedia.org/w/index.php?title=Special:Storyboard&id=123 and use $wgRequest->getIntOrNull( 'id' ) to grab the ID.

Yes, array( 'story_id' => $params['storyid'] ) works and should be used.

Yes, it also looks like selectRow() is the way to go here.

+			$this->dieUsageMsg( array( 'storyreview' ) );

This doesn't magically work, you need to add the error message to ApiBase::$messageMap as well. I suggest you use an existing message from that array, there's definitely stuff in there for permission errors.

+			'storyaction' => null,

This should be something along the lines of:

'storyaction' => array(
	ApiBase::PARAM_TYPE => array(
		'hide',
		'unhide',
		...
	)
)

You can also set ApiBase::PARAM_ISMULTI => true to make this parameter multivalued, so setting &storyaction=unhide|publish will result in $params['storyaction'] being array( 'unhide', 'publish' ). In this case, you do have to look out for invalid/bogus combinations such as hide|unhide or delete|publish.

#Comment by Jeroen De Dauw (talk | contribs) 00:21, 15 March 2010

Thanks guys! Fixed stuff in r63758.

Status & tagging log

14:57, 15 March 2010 Catrope (talk | contribs) changed the status of r63722 [removed: new added: resolved]