r106404 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r106403‎ \| r106404 \| r106405 >
Date:	02:24, 16 December 2011
Author:	jeroendedauw
Status:	deferred (Comments)
Tags:	educationprogram
Comment:	unfinished code, will follow up soonish, from other device
Modified paths:	/trunk/extensions/EducationProgram/EducationProgram.i18n.php (modified) (history) /trunk/extensions/EducationProgram/EducationProgram.php (modified) (history) /trunk/extensions/EducationProgram/specials/SpecialEPFormPage.php (added) (history) /trunk/extensions/EducationProgram/specials/SpecialEPPage.php (modified) (history) /trunk/extensions/EducationProgram/specials/SpecialEditInstitution.php (added) (history)

Diff [purge]

Index: trunk/extensions/EducationProgram/specials/SpecialEditInstitution.php
—	—	@@ -0,0 +1,305 @@
	2	+<?php
	3	+
	4	+/**
	5	+ * Adittion and modification interface for insitutions.
	6	+ *
	7	+ * @since 0.1
	8	+ *
	9	+ * @file SpecialEditInstitution.php
	10	+ * @ingroup EducationProgram
	11	+ *
	12	+ * @licence GNU GPL v3 or later
	13	+ * @author Jeroen De Dauw < jeroendedauw@gmail.com >
	14	+ */
	15	+class SpecialEditInstitution extends SpecialEPFormPage {
	16	+
	17	+ /**
	18	+ * Constructor.
	19	+ *
	20	+ * @since 0.1
	21	+ */
	22	+ public function __construct() {
	23	+ parent::__construct( 'EditInstitution', 'epadmin', false );
	24	+ }
	25	+
	26	+ /**
	27	+ * Attempt to get the contest to be edited or create the one to be added.
	28	+ * If this works, show the form, if not, redirect to special:contests.
	29	+ *
	30	+ * @since 0.1
	31	+ */
	32	+ protected function showContent() {
	33	+ if ( $this->isNew() ) {
	34	+ $conditions = array( 'name' => $this->getRequest()->getVal( 'neworg' ) );
	35	+
	36	+ $contest = Contest::s()->selectRow( null, $conditions );
	37	+
	38	+ if ( $contest === false ) {
	39	+ $contest = new Contest( $conditions, true );
	40	+ }
	41	+ else {
	42	+ $this->showWarning( 'contest-edit-exists-already' );
	43	+ }
	44	+ }
	45	+ else {
	46	+ $contest = Contest::s()->selectRow( null, array( 'name' => $this->subPage ) );
	47	+ }
	48	+
	49	+ if ( $contest === false ) {
	50	+ $this->getOutput()->redirect( SpecialPage::getTitleFor( 'Contests' )->getLocalURL() );
	51	+ }
	52	+ else {
	53	+ if ( !$this->isNew() ) {
	54	+ $this->getOutput()->addHTML(
	55	+ SpecialContestPage::getNavigation( $contest->getField( 'name' ), $this->getUser(), $this->getLanguage(), $this->getName() )
	56	+ );
	57	+ }
	58	+
	59	+ $this->contest = $contest;
	60	+ $this->showForm();
	61	+ $this->getOutput()->addModules( 'contest.special.editcontest' );
	62	+ }
	63	+ }
	64	+
	65	+ /**
	66	+ * (non-PHPdoc)
	67	+ * @see FormSpecialPage::getFormFields()
	68	+ * @return array
	69	+ */
	70	+ protected function getFormFields() {
	71	+
	72	+ /**
	73	+ * @var $contest Contest
	74	+ */
	75	+ $contest = $this->contest;
	76	+
	77	+ $fields = array();
	78	+
	79	+ $fields['id'] = array ( 'type' => 'hidden' );
	80	+
	81	+ $fields['name'] = array (
	82	+ 'type' => 'text',
	83	+ 'label-message' => 'contest-edit-name',
	84	+ 'id' => 'contest-name-field',
	85	+ );
	86	+
	87	+ $fields['status'] = array (
	88	+ 'type' => 'radio',
	89	+ 'label-message' => 'contest-edit-status',
	90	+ 'options' => Contest::getStatusMessages( true )
	91	+ );
	92	+
	93	+ $fields['intro'] = array (
	94	+ 'type' => 'text',
	95	+ 'label-message' => 'contest-edit-intro',
	96	+ );
	97	+
	98	+ $fields['opportunities'] = array (
	99	+ 'type' => 'text',
	100	+ 'label-message' => 'contest-edit-opportunities',
	101	+ );
	102	+
	103	+ $fields['rules_page'] = array (
	104	+ 'type' => 'text',
	105	+ 'label-message' => 'contest-edit-rulespage',
	106	+ );
	107	+
	108	+ $fields['help'] = array (
	109	+ 'type' => 'text',
	110	+ 'label-message' => 'contest-edit-help',
	111	+ );
	112	+
	113	+ $fields['signup_email'] = array (
	114	+ 'type' => 'text',
	115	+ 'label-message' => 'contest-edit-signup',
	116	+ );
	117	+
	118	+ $fields['reminder_email'] = array (
	119	+ 'type' => 'text',
	120	+ 'label-message' => 'contest-edit-reminder',
	121	+ );
	122	+
	123	+ $fields['end'] = array (
	124	+ 'type' => 'text',
	125	+ 'label-message' => 'contest-edit-end',
	126	+ 'id' => 'contest-edit-end',
	127	+ 'size' => 15
	128	+ );
	129	+
	130	+ if ( $contest !== false ) {
	131	+ foreach ( $fields as $name => $data ) {
	132	+ $default = $contest->getField( $name );
	133	+
	134	+ if ( $name == 'end' ) {
	135	+ $default = wfTimestamp( TS_DB, $default );
	136	+ }
	137	+
	138	+ $fields[$name]['default'] = $default;
	139	+ }
	140	+ }
	141	+
	142	+ $mappedFields = array();
	143	+
	144	+ foreach ( $fields as $name => $field ) {
	145	+ $mappedFields['contest-' . $name] = $field;
	146	+ }
	147	+
	148	+ if ( $contest !== false ) {
	149	+ foreach ( $contest->getChallenges() as /* ContestChallenge */ $challenge ) {
	150	+ $mappedFields[] = array(
	151	+ 'class' => 'ContestChallengeField',
	152	+ 'options' => $challenge->toArray()
	153	+ );
	154	+ }
	155	+ }
	156	+
	157	+ $mappedFields['delete-challenges'] = array ( 'type' => 'hidden', 'id' => 'delete-challenges' );
	158	+
	159	+ return $mappedFields;
	160	+ }
	161	+
	162	+ /**
	163	+ * Process the form. At this point we know that the user passes all the criteria in
	164	+ * userCanExecute(), and if the data array contains 'Username', etc, then Username
	165	+ * resets are allowed.
	166	+ *
	167	+ * @param array $data
	168	+ *
	169	+ * @return Bool\|Array
	170	+ */
	171	+ public function onSubmit( array $data ) {
	172	+ $fields = array();
	173	+
	174	+ foreach ( $data as $name => $value ) {
	175	+ $matches = array();
	176	+
	177	+ if ( preg_match( '/contest-(.+)/', $name, $matches ) ) {
	178	+ if ( $matches[1] == 'end' ) {
	179	+ $value = wfTimestamp( TS_MW, strtotime( $value ) );
	180	+ }
	181	+
	182	+ $fields[$matches[1]] = $value;
	183	+ }
	184	+ }
	185	+
	186	+ // If no ID is set, this means it's a new contest, so set the ID to null for an insert.
	187	+ // However, the user can have hot the back button after creation of a new contest,
	188	+ // re-submitting the form. In this case, get the ID of the already existing item for an update.
	189	+ if ( !array_key_exists( 'id', $fields ) \|\| $fields['id'] === '' ) {
	190	+ $contest = Contest::s()->selectRow( 'id', array( 'name' => $fields['name'] ) );
	191	+ $fields['id'] = $contest === false ? null : $contest->getField( 'id' );
	192	+ }
	193	+
	194	+ $contest = new Contest( $fields, is_null( $fields['id'] ) );
	195	+
	196	+ $contest->setChallenges( $this->getSubmittedChallenges() );
	197	+ $success = $contest->writeAllToDB();
	198	+
	199	+ $success = $this->removeDeletedChallenges( $data['delete-challenges'] ) && $success;
	200	+
	201	+ if ( $success ) {
	202	+ return true;
	203	+ }
	204	+ else {
	205	+ return array(); // TODO
	206	+ }
	207	+ }
	208	+
	209	+ /**
	210	+ * The UI keeps track of 'removed' challenges by storing them into a
	211	+ * hidden HTML input, pipe-separated. On submission, this method
	212	+ * takes this string and actually deletes them.
	213	+ *
	214	+ * @since 0.1
	215	+ *
	216	+ * @param string $idString
	217	+ *
	218	+ * @return boolean Success indicator
	219	+ */
	220	+ protected function removeDeletedChallenges( $idString ) {
	221	+ if ( $idString == '' ) {
	222	+ return true;
	223	+ }
	224	+
	225	+ if ( !ContestSettings::get( 'contestDeletionEnabled' ) ) {
	226	+ // Shouldn't get here (UI should prevent it)
	227	+ throw new MWException( 'Contest deletion is disabled', 'contestdeletiondisabled' );
	228	+ }
	229	+
	230	+ return ContestChallenge::s()->delete( array( 'id' => explode( '\|', $idString ) ) );
	231	+ }
	232	+
	233	+ /**
	234	+ * Finds the submitted challanges and returns them as a list of
	235	+ * ContestChallenge objects.
	236	+ *
	237	+ * @since 0.1
	238	+ *
	239	+ * @return array of ContestChallenge
	240	+ */
	241	+ protected function getSubmittedChallenges() {
	242	+ $challenges = array();
	243	+
	244	+ foreach ( $this->getrequest()->getValues() as $name => $value ) {
	245	+ $matches = array();
	246	+
	247	+ if ( preg_match( '/contest-challenge-(\d+)/', $name, $matches ) ) {
	248	+ $challenges[] = $this->getSubmittedChallenge( $matches[1] );
	249	+ } elseif ( preg_match( '/contest-challenge-new-(\d+)/', $name, $matches ) ) {
	250	+ $challenges[] = $this->getSubmittedChallenge( $matches[1], true );
	251	+ }
	252	+ }
	253	+
	254	+ return $challenges;
	255	+ }
	256	+
	257	+ /**
	258	+ * Create and return a contest challenge object from the submitted data.
	259	+ *
	260	+ * @since 0.1
	261	+ *
	262	+ * @param integer\|null $challengeId
	263	+ *
	264	+ * @return ContestChallenge
	265	+ */
	266	+ protected function getSubmittedChallenge( $challengeId, $isNewChallenge = false ) {
	267	+ if ( $isNewChallenge ) {
	268	+ $challengeDbId = null;
	269	+ $challengeId = "new-$challengeId";
	270	+ } else {
	271	+ $challengeDbId = $challengeId;
	272	+ }
	273	+
	274	+ $request = $this->getRequest();
	275	+
	276	+ return new ContestChallenge( array(
	277	+ 'id' => $challengeDbId,
	278	+ 'text' => $request->getText( "challenge-text-$challengeId" ),
	279	+ 'title' => $request->getText( "contest-challenge-$challengeId" ),
	280	+ 'oneline' => $request->getText( "challenge-oneline-$challengeId" ),
	281	+ ) );
	282	+ }
	283	+
	284	+ /**
	285	+ * Gets called after the form is saved.
	286	+ * Enter description here ...
	287	+ */
	288	+ public function onSuccess() {
	289	+ $this->getOutput()->redirect( SpecialPage::getTitleFor( 'Institutions' )->getLocalURL() );
	290	+ }
	291	+
	292	+ /**
	293	+ * Show a message in a warning box.
	294	+ *
	295	+ * @since 0.1
	296	+ *
	297	+ * @param string $message
	298	+ */
	299	+ protected function showWarning( $message ) {
	300	+ $this->getOutput()->addHTML(
	301	+ '<p class="visualClear warningbox">' . wfMsgExt( $message, 'parseinline' ) . '</p>'
	302	+ . '<hr style="display: block; clear: both; visibility: hidden;" />'
	303	+ );
	304	+ }
	305	+
	306	+}
Index: trunk/extensions/EducationProgram/specials/SpecialEPPage.php
—	—	@@ -14,6 +14,13 @@
15	15	*/
16	16	abstract class SpecialEPPage extends SpecialPage {
17	17
	18	+ /**
	19	+ * The subpage, ie the part after Special:PageName/
	20	+ * Emptry string if none is provided.
	21	+ *
	22	+ * @since 0.1
	23	+ * @var string
	24	+ */
18	25	public $subPage;
19	26
20	27	/**
Index: trunk/extensions/EducationProgram/specials/SpecialEPFormPage.php
—	—	@@ -0,0 +1,135 @@
	2	+<?php
	3	+
	4	+/**
	5	+ * Extends FormSpecialPage with commons functions needed in EducationProgram.
	6	+ *
	7	+ * @since 0.1
	8	+ *
	9	+ * @file SpecialEPFormPage.php
	10	+ * @ingroup EducationProgram
	11	+ *
	12	+ * @licence GNU GPL v3 or later
	13	+ * @author Jeroen De Dauw < jeroendedauw@gmail.com >
	14	+ */
	15	+abstract class SpecialEPFormPage extends FormSpecialPage {
	16	+
	17	+ /**
	18	+ * The subpage, ie the part after Special:PageName/
	19	+ * Emptry string if none is provided.
	20	+ *
	21	+ * @since 0.1
	22	+ * @var string
	23	+ */
	24	+ public $subPage;
	25	+
	26	+ /**
	27	+ * @see SpecialPage::getDescription
	28	+ *
	29	+ * @since 0.1
	30	+ */
	31	+ public function getDescription() {
	32	+ $action = $this->isNew() ? 'add' : 'edit';
	33	+ return wfMsg( 'special-' . strtolower( $this->getName() ) . $action );
	34	+ }
	35	+
	36	+ /**
	37	+ * Sets headers - this should be called from the execute() method of all derived classes!
	38	+ *
	39	+ * @since 0.1
	40	+ */
	41	+ public function setHeaders() {
	42	+ $out = $this->getOutput();
	43	+ $out->setArticleRelated( false );
	44	+ $out->setRobotPolicy( 'noindex,nofollow' );
	45	+ $out->setPageTitle( $this->getDescription() );
	46	+ }
	47	+
	48	+ /**
	49	+ * Main method.
	50	+ *
	51	+ * @since 0.1
	52	+ *
	53	+ * @param string $subPage
	54	+ */
	55	+ public function execute( $subPage ) {
	56	+ $subPage = is_null( $subPage ) ? '' : $subPage;
	57	+ $this->subPage = trim( str_replace( '_', ' ', $subPage ) );
	58	+
	59	+ $this->setHeaders();
	60	+ $this->outputHeader();
	61	+
	62	+ // This will throw exceptions if there's a problem.
	63	+ $this->checkExecutePermissions( $this->getUser() );
	64	+
	65	+ if ( $this->isNew() ) {
	66	+ $this->showForm();
	67	+ }
	68	+ else {
	69	+ $this->showContent();
	70	+ }
	71	+ }
	72	+
	73	+ /**
	74	+ *
	75	+ * Enter description here ...
	76	+ */
	77	+ protected function isNew() {
	78	+ return $this->getRequest()->wasPosted() && $this->getUser()->matchEditToken( $this->getRequest()->getVal( 'wpEditToken' ) );
	79	+ }
	80	+
	81	+ /**
	82	+ * Show the form.
	83	+ *
	84	+ * @since 0.1
	85	+ */
	86	+ protected function showForm() {
	87	+ $form = $this->getForm();
	88	+
	89	+ if ( $form->show() ) {
	90	+ $this->onSuccess();
	91	+ }
	92	+ }
	93	+
	94	+ /**
	95	+ * (non-PHPdoc)
	96	+ * @see FormSpecialPage::getForm()
	97	+ * @return HTMLForm\|null
	98	+ */
	99	+ protected function getForm() {
	100	+ $form = parent::getForm();
	101	+
	102	+ $form->addButton(
	103	+ 'cancelEdit',
	104	+ wfMsg( 'cancel' ),
	105	+ 'cancelEdit',
	106	+ array(
	107	+ 'target-url' => SpecialPage::getTitleFor( 'Contests' )->getFullURL()
	108	+ )
	109	+ );
	110	+
	111	+// $form->addButton(
	112	+// 'deleteEdit',
	113	+// wfMsg( 'delete' ),
	114	+// 'deleteEdit'
	115	+// );
	116	+
	117	+ return $form;
	118	+ }
	119	+
	120	+ /**
	121	+ * Show a message in a warning box.
	122	+ *
	123	+ * @since 0.1
	124	+ *
	125	+ * @param string $message Message key
	126	+ * @param array\|string $args Message arguments
	127	+ */
	128	+ protected function showWarning( $message, $args = array() ) {
	129	+ $message = call_user_func_array( 'wfMsgExt', array_merge( array( $message ), (array)$args ) );
	130	+ $this->getOutput()->addHTML(
	131	+ '<p class="visualClear warningbox">' . $message . '</p>'
	132	+ . '<hr style="display: block; clear: both; visibility: hidden;" />'
	133	+ );
	134	+ }
	135	+
	136	+}
Index: trunk/extensions/EducationProgram/EducationProgram.i18n.php
—	—	@@ -53,6 +53,8 @@
54	54	'special-course' => 'Course',
55	55	'special-courses' => 'Courses',
56	56	'special-educationprogram' => 'Education Program',
	57	+ 'special-editinstitution-add' => 'Add institution',
	58	+ 'special-editinstitution-edit' => 'Edit institution',
57	59
58	60	// Special:Institutions
59	61	'ep-institutions-nosuchinstitution' => 'There is no institution with name "$1". Existing institutions are listed below.',
Index: trunk/extensions/EducationProgram/EducationProgram.php
—	—	@@ -62,6 +62,7 @@
63	63
64	64	$wgAutoloadClasses['SpecialCourse'] = dirname( __FILE__ ) . '/specials/SpecialCourse.php';
65	65	$wgAutoloadClasses['SpecialCourses'] = dirname( __FILE__ ) . '/specials/SpecialCourses.php';
	66	+$wgAutoloadClasses['SpecialEditInstitution'] = dirname( __FILE__ ) . '/specials/SpecialEditInstitution.php';
66	67	$wgAutoloadClasses['SpecialEducationProgram'] = dirname( __FILE__ ) . '/specials/SpecialEducationProgram.php';
67	68	$wgAutoloadClasses['SpecialEPPage'] = dirname( __FILE__ ) . '/specials/SpecialEPPage.php';
68	69	$wgAutoloadClasses['SpecialInstitution'] = dirname( __FILE__ ) . '/specials/SpecialInstitution.php';

Follow-up revisions

Revision	Commit summary	Author	Date
r106405	follow up to r106404 - created base class for form pages taking away most of ...	jeroendedauw	03:12, 16 December 2011
r106407	follow up to r106404 r106405 - cleanup and fixes	jeroendedauw	03:34, 16 December 2011
r106595	follow up to r106404, use wfMessage and Message class as Nikerabbit suggested	jeroendedauw	20:18, 18 December 2011

Comments

#Comment by Nikerabbit (talk | contribs) 10:20, 16 December 2011

+		parent::__construct( 'EditInstitution', 'epadmin', false );

Can you use a setter instead of boolean parameter in the constructor? It's not readable.

#Comment by Jeroen De Dauw (talk | contribs) 18:42, 16 December 2011

Err. This is how I've always done it in SpecialPages... how do you suggest doing it instead?

#Comment by Nikerabbit (talk | contribs) 19:45, 18 December 2011

$this->setListed( true ) in the constructor?

#Comment by Jeroen De Dauw (talk | contribs) 20:12, 18 December 2011

I actually dislike that quite a bit. Passing such arguments to the constructor seems a lot nicer. PHP just fails for not being able to have named args such as Python or a sane dictionary syntax such as JS. Then again, if you got a decent editor, it ought to just show what the stuff means by looking at the function definition and it's corresponding comments.

#Comment by Nikerabbit (talk | contribs) 20:15, 18 December 2011

If you dislike that, how about doing:

parent::__construct( 'EditInstitution', 'epadmin', /*listed*/false );

$listed = false;
parent::__construct( 'EditInstitution', 'epadmin', $listed );

#Comment by Jeroen De Dauw (talk | contribs) 20:23, 18 December 2011

/*str*/ $arg

is used for type hinting, so doing it with a constant and using it as meaning indicator would probably be confusing.

Using separate vars just makes the code messy. There is no need to do this. And this argument does not only apply to constructors, it applies to all functions where it's not obvious what some argument might be, which are pretty much all those that are not setFoo and have only a single argument.

Seriously, such documentation should go with the definition of the function and be inferred from that by your editor.

#Comment by Nikerabbit (talk | contribs) 20:26, 18 December 2011

Very few of us are using more than text editor with syntax highlighting to program.

#Comment by Jeroen De Dauw (talk | contribs) 20:47, 18 December 2011

Sure.

wfMessage( /* message key */ 'bunny', /* argument $1 */ $toy, /* argument $2 */ $car )

Html::element( /* tag type */ 'div', /* attributes */ array(), /* tag content */ 'meh' )

If everyone does that through all core code and extensions, I'll happily do it as well.

#Comment by Nikerabbit (talk | contribs) 10:21, 16 December 2011

Could using Message objects simplify showWarning? It's more versatile that way anyway.

#Comment by Jeroen De Dauw (talk | contribs) 18:42, 16 December 2011

Again, can you provide some example code?

#Comment by Nikerabbit (talk | contribs) 20:01, 18 December 2011

function showWarning( Message $message ) {
  return ..... $message->parse() ....;
}

somewhere else:

$this->showWarning( wfMessage( 'bunny', $toy, $car ) );

Status & tagging log

11:13, 5 January 2012 Hashar (talk | contribs) changed the status of r106404 [removed: new added: deferred]
03:18, 23 December 2011 RobLa-WMF (talk | contribs) changed the tags for r106404 [added: educationprogram]