r111666 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r111665‎ \| r111666 \| r111667 >
Date:	19:44, 16 February 2012
Author:	jeroendedauw
Status:	deferred (Comments)
Tags:
Comment:	tweaks to logging
Modified paths:	/trunk/extensions/EducationProgram/EducationProgram.i18n.php (modified) (history) /trunk/extensions/EducationProgram/api/ApiEnlist.php (modified) (history) /trunk/extensions/EducationProgram/includes/EPCourse.php (modified) (history) /trunk/extensions/EducationProgram/specials/SpecialDisenroll.php (modified) (history)

Diff [purge]

Index: trunk/extensions/EducationProgram/specials/SpecialDisenroll.php
—	—	@@ -163,10 +163,15 @@
164	164	* @param EPCourse $course
165	165	*/
166	166	protected function doDisenroll( EPCourse $course ) {
	167	+ $revAction = new EPRevisionAction();
	168	+ $revAction->setUser( $this->getUser() );
	169	+ $revAction->setComment( $this->getRequest()->getText( 'summary' ) );
	170	+
167	171	$success = $course->unenlistUsers(
168	172	$this->getUser()->getId(),
169	173	'student',
170		~~- $this->getRequest()->getText( 'summary' )~~
	174	+ true,
	175	+ $revAction
171	176	);
172	177
173	178	if ( $success ) {
Index: trunk/extensions/EducationProgram/includes/EPCourse.php
—	—	@@ -548,13 +548,12 @@
549	549	*
550	550	* @param array\|integer $newUsers
551	551	* @param string $role
552		~~- * @param string $message~~
553	552	* @param boolean $save
554		~~- * @param boolean $log~~
	553	+ * @param EPRevisionAction\|null $revAction
555	554	*
556	555	* @return boolean Success indicator
557	556	*/
558		~~- public function enlistUsers( $newUsers, $role, $message = '', $save = true, $log = true ) {~~
	557	+ public function enlistUsers( $newUsers, $role, $save = true, EPRevisionAction $revAction = null ) {
559	558	$roleMap = array(
560	559	'student' => 'students',
561	560	'campus' => 'campus_ambs',
—	—	@@ -588,8 +587,9 @@
589	588	$this->enableLogging();
590	589	}
591	590
592		~~- if ( $success && $log ) {~~
593		~~- $this->logRoleChange( 'add', $role, $addedUsers, $message );~~
	591	+ if ( $success && !is_null( $revAction ) ) {
	592	+ $action = count( $addedUsers ) == 1 && $revAction->getUser()->getId() === $addedUsers[0] ? 'selfadd' : 'add';
	593	+ $this->logRoleChange( $action, $role, $addedUsers, $revAction->getComment() );
594	594	}
595	595
596	596	return $success;
—	—	@@ -608,14 +608,13 @@
609	609	*
610	610	* @param array\|integer $sadUsers
611	611	* @param string $role
612		~~- * @param string $message~~
613	612	* @param boolean $save
614		~~- * @param boolean $log~~
	613	+ * @param EPRevisionAction\|null $revAction
615	614	*
616	615	* @return boolean Success indicator
617	616	*/
618		~~- public function unenlistUsers( $sadUsers, $role, $message = '', $save = true, $log = true ) {~~
619		~~- $removedUser = array();~~
	617	+ public function unenlistUsers( $sadUsers, $role, $save = true, EPRevisionAction $revAction = null ) {
	618	+ $removedUsers = array();
620	619	$remaimingUsers = array();
621	620	$sadUsers = (array)$sadUsers;
622	621
—	—	@@ -630,14 +629,14 @@
631	630
632	631	foreach ( $this->getField( $field ) as $userId ) {
633	632	if ( in_array( $userId, $sadUsers ) ) {
634		~~- $removedUser[] = $userId;~~
	633	+ $removedUsers[] = $userId;
635	634	}
636	635	else {
637	636	$remaimingUsers[] = $userId;
638	637	}
639	638	}
640	639
641		~~- if ( count( $removedUser ) > 0 ) {~~
	640	+ if ( count( $removedUsers ) > 0 ) {
642	641	$this->setField( $field, $remaimingUsers );
643	642
644	643	$success = true;
—	—	@@ -648,8 +647,9 @@
649	648	$this->enableLogging();
650	649	}
651	650
652		~~- if ( $success && $log ) {~~
653		~~- $this->logRoleChange( 'remove', $role, $removedUser, $message );~~
	651	+ if ( $success && !is_null( $revAction ) ) {
	652	+ $action = count( $removedUsers ) == 1 && $revAction->getUser()->getId() === $removedUsers[0] ? 'selfremove' : 'remove';
	653	+ $this->logRoleChange( $action, $role, $removedUsers, $revAction->getComment() );
654	654	}
655	655
656	656	return $success;
—	—	@@ -690,8 +690,8 @@
691	691	'subtype' => $action,
692	692	'title' => $this->getTitle(),
693	693	'parameters' => array(
694		~~- '4::instructorcount' => count( $names ),~~
695		~~- '5::instructors' => $GLOBALS['wgLang']->listToText( $names )~~
	694	+ '4::usercount' => count( $names ),
	695	+ '5::users' => $GLOBALS['wgLang']->listToText( $names )
696	696	),
697	697	);
698	698
Index: trunk/extensions/EducationProgram/EducationProgram.i18n.php
—	—	@@ -94,7 +94,7 @@
95	95	'logentry-instructor-add' => '$1 {{GENDER:$2\|added}} {{PLURAL:$4\|instructor\|instructors}} $5 to course $3',
96	96	'logentry-instructor-remove' => '$1 {{GENDER:$2\|removed}} {{PLURAL:$4\|instructor\|instructors}} $5 from course $3',
97	97	'logentry-instructor-selfadd' => '$1 added {{GENDER:$2\|added himself\|herself}} as {{GENDER:$2\|instructor}} to course $3',
98		~~- 'logentry-instructor-selfremove' => '$1 removed {{GENDER:$2\|removed himself\|herself}} as {{GENDER:$2\|instructor}} from course $3',~~
	98	+ 'logentry-instructor-selfremove' => '$1 removed {{GENDER:$2\|himself\|herself}} as {{GENDER:$2\|instructor}} from course $3',
99	99
100	100	'logentry-online-add' => '$1 added {{PLURAL:$4\|Online Ambassador\|Online Ambassadors}} $5 to course $3',
101	101	'logentry-online-remove' => '$1 removed {{PLURAL:$4\|Online Ambassador\|Online Ambassadors}} $5 from course $3',
—	—	@@ -107,7 +107,7 @@
108	108	'logentry-campus-selfremove' => '$1 removed {{GENDER:$2\|himself\|herself}} as {{GENDER:$2\|Campus Ambassador}} from course $3',
109	109
110	110	'logentry-student-add' => '$1 enrolled in course $3',
111		~~- 'logentry-student-remove' => '$1 removed $4 as student from course $3',~~
	111	+ 'logentry-student-remove' => '$1 removed $5 as student from course $3',
112	112	'logentry-student-selfremove' => '$1 removed {{GENDER:$2\|his\|her}} enrollment from course $3',
113	113
114	114	// Preferences
Index: trunk/extensions/EducationProgram/api/ApiEnlist.php
—	—	@@ -53,13 +53,17 @@
54	54	}
55	55
56	56	$success = false;
57		-
	57	+
	58	+ $revAction = new EPRevisionAction();
	59	+ $revAction->setUser( $this->getUser() );
	60	+ $revAction->setComment( $params['reason'] );
	61	+
58	62	switch ( $params['subaction'] ) {
59	63	case 'add':
60		~~- $success = $course->enlistUsers( array( $userId ), $params['role'], $params['reason'] );~~
	64	+ $success = $course->enlistUsers( array( $userId ), $params['role'], true, $revAction );
61	65	break;
62	66	case 'remove':
63		~~- $success = $course->unenlistUsers( array( $userId ), $params['role'], $params['reason'] );~~
	67	+ $success = $course->unenlistUsers( array( $userId ), $params['role'], true, $revAction );
64	68	break;
65	69	}
66	70

Follow-up revisions

Revision	Commit summary	Author	Date
r111793	follow up to r111666	jeroendedauw	21:27, 17 February 2012

Comments

#Comment by Nikerabbit (talk | contribs) 06:39, 17 February 2012

'5::users' => $GLOBALS['wgLang']->listToText( $names )

You probably don't want to add preformatted data to logging table depending on the current user's language.

#Comment by Dantman (talk | contribs) 06:49, 17 February 2012

Not to mention that the request context should be used.

Status & tagging log

21:46, 17 February 2012 😂 (talk | contribs) changed the status of r111666 [removed: new added: deferred]