r104644 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r104643‎ \| r104644 \| r104645 >
Date:	01:42, 30 November 2011
Author:	bsitu
Status:	resolved (Comments)
Tags:
Comment:	followup to r104642 - Moodbar Phase 2 server side change
Modified paths:	/trunk/extensions/MoodBar/ApiFeedbackDashboardResponse.php (modified) (history) /trunk/extensions/MoodBar/ApiQueryMoodBarComments.php (modified) (history) /trunk/extensions/MoodBar/MoodBar.i18n.php (modified) (history) /trunk/extensions/MoodBar/SpecialFeedbackDashboard.php (modified) (history)

Diff [purge]

Index: trunk/extensions/MoodBar/SpecialFeedbackDashboard.php
—	—	@@ -128,9 +128,10 @@
129	129	* @param $params An array of flags. Valid flags:
130	130	* * admin (user can show/hide feedback items)
131	131	* * show-anyway (user has asked to see this hidden item)
	132	+ * @param $response An array of response for feedback
132	133	* @return string HTML
133	134	*/
134		~~- public static function formatListItem( $row, $params = array() ) {~~
	135	+ public static function formatListItem( $row, $params = array(), $response = array() ) {
135	136	global $wgLang, $wgUser;
136	137
137	138	$classes = array('fbd-item');
—	—	@@ -142,6 +143,7 @@
143	144	$feedbackItem = MBFeedbackItem::load( $row );
144	145	}
145	146	catch (Exception $e) {
	147	+ $classes = Sanitizer::encodeAttribute( implode(' ', $classes) );
146	148	$error_message = wfMessage('moodbar-feedback-load-record-error')->escaped();
147	149	return <<<HTML
148	150	<li class="$classes">
—	—	@@ -200,22 +202,10 @@
201	203
202	204	}
203	205
204		~~- //only show response elements if feedback is not hidden, and user is logged in~~
205		~~- if ($feedbackItem->getProperty('hidden-state') == false~~
206		~~- && !$wgUser->isAnon() ) {~~
207		~~- $respondToThis = "<span>".wfMessage('moodbar-respond-collapsed')->escaped().'</span> '.wfMessage("moodbar-respond-text")->escaped();~~
208		~~- $responseElements = <<<HTML~~
209		~~- <div class="fbd-item-response">~~
210		~~- <a class="fbd-respond-link">$respondToThis</a>~~
211		~~- </div>~~
212		~~-HTML;~~
213		~~- }~~
	206	+ $responseElements = self::buildResponseElement( $feedbackItem, $response );
214	207
215	208	$classes = Sanitizer::encodeAttribute( implode(' ', $classes) );
216	209	$toolLinks = implode("\n", $toolLinks );
217		~~- if (!isset($responseElements)) {~~
218		~~- $responseElements = "";~~
219		~~- }~~
220	210
221	211	return <<<HTML
222	212	<li class="$classes" data-mbccontinue="$continueData">
—	—	@@ -232,6 +222,63 @@
233	223	HTML;
234	224	}
235	225
	226	+ protected static function buildResponseElement( $feedbackItem, $response ) {
	227	+ global $wgLang, $wgUser;
	228	+
	229	+ $responseElements = '';
	230	+
	231	+ $id = $feedbackItem->getProperty('id');
	232	+
	233	+ $showResponseBox = true;
	234	+
	235	+ //Do not show response box if there is a response already
	236	+ if ( isset( $response[$id] ) ) {
	237	+ //for now we only display the latest response
	238	+ foreach ( $response[$id] AS $response_detail ) {
	239	+ if ( $responder = User::newFromId( $response_detail->mbfr_user_id ) ) {
	240	+
	241	+ $now = wfTimestamp( TS_UNIX );
	242	+ $responsetimestamp = wfTimestamp( TS_UNIX, $response_detail->mbfr_timestamp );
	243	+
	244	+ $responsetime = $wgLang->formatTimePeriod( $now - $responsetimestamp,
	245	+ array( 'avoid' => 'avoidminutes', 'noabbrevs' => true )
	246	+ );
	247	+
	248	+ $permalinkTitle = $feedbackItem->getProperty('user')->getTalkPage()->getFullText();
	249	+
	250	+ $individual_response = wfMsgExt('moodbar-feedback-response-summary', array('parse'),
	251	+ $responder->getUserPage()->getFullText(),
	252	+ htmlspecialchars($responder->getName()),
	253	+ $permalinkTitle . '#feedback-dashboard-response-' . $response_detail->mbfr_id,
	254	+ $responsetime);
	255	+ $showResponseBox = false;
	256	+
	257	+ $responseElements = <<<HTML
	258	+ <div class="fbd-item-response">
	259	+ $individual_response
	260	+ </div>
	261	+HTML;
	262	+ break;
	263	+
	264	+ }
	265	+ }
	266	+
	267	+ }
	268	+ //only show response elements if feedback is not hidden, and user is logged in
	269	+ else if ( $showResponseBox && $feedbackItem->getProperty('hidden-state') == false
	270	+ && !$wgUser->isAnon() ) {
	271	+ $respondToThis = "<span>".wfMessage('moodbar-respond-collapsed')->escaped().'</span> '.wfMessage("moodbar-respond-text")->escaped();
	272	+ $responseElements = <<<HTML
	273	+ <div class="fbd-item-response">
	274	+ <a class="fbd-respond-link">$respondToThis</a>
	275	+ </div>
	276	+HTML;
	277	+ }
	278	+
	279	+ return $responseElements;
	280	+
	281	+ }
	282	+
236	283	/**
237	284	* Build the "user information" part of an item on the feedback dashboard.
238	285	* @param $feedbackItem MBFeedbackItem representing the feedback to show
—	—	@@ -350,8 +397,10 @@
351	398	}
352	399	}
353	400
	401	+ $response = self::getResponseSummary( $res['rows'] );
	402	+
354	403	foreach ( $res['rows'] as $row ) {
355		~~- $list .= self::formatListItem( $row, $params );~~
	404	+ $list .= self::formatListItem( $row, $params, $response );
356	405	}
357	406
358	407	if ( $list === '' ) {
—	—	@@ -555,4 +604,37 @@
556	605	);
557	606	}
558	607
	608	+ /**
	609	+ * Get the response summary for a set of feedback
	610	+ * @param $res Iterator of Db row with index mbf_id for feedback
	611	+ * @return array
	612	+ */
	613	+ public static function getResponseSummary( $res ) {
	614	+ $dbr = wfGetDB( DB_SLAVE );
	615	+
	616	+ $feedback = array();
	617	+
	618	+ foreach ( $res as $row ) {
	619	+ $feedback[] = $row->mbf_id;
	620	+ }
	621	+
	622	+ $response = array();
	623	+
	624	+ if ( $feedback = implode( ',', $feedback ) ) {
	625	+ $res = $dbr->select( array( 'moodbar_feedback_response' ),
	626	+ array( 'mbfr_id', 'mbfr_mbf_id', 'mbfr_user_id', 'mbfr_timestamp' ),
	627	+ array( 'mbfr_mbf_id IN (' . $feedback . ') AND mbfr_user_id != 0' ),
	628	+ __METHOD__,
	629	+ array( 'ORDER BY' => "mbfr_timestamp DESC, mbfr_id DESC" )
	630	+ );
	631	+
	632	+ foreach ( $res AS $row ) {
	633	+ $response[$row->mbfr_mbf_id][] = $row;
	634	+ }
	635	+ }
	636	+
	637	+
	638	+ return $response;
	639	+ }
	640	+
559	641	}
Index: trunk/extensions/MoodBar/MoodBar.i18n.php
—	—	@@ -80,7 +80,7 @@
81	81	'moodbar-header-own-talk' => 'Own talk page',
82	82	// Special:MoodBarFeedback
83	83	'moodbar-feedback-title' => 'Feedback dashboard',
84		~~- 'moodbar-feedback-response-title' => 'Feedback dashboard response',~~
	84	+ 'moodbar-feedback-response-title' => '==In response to your [[$1\|feedback]]==',
85	85	'moodbar-feedback-view-link' => '(View the feedback)',
86	86	'moodbar-feedback-filters' => 'Filters',
87	87	'moodbar-feedback-filters-type' => 'Mood:',
—	—	@@ -116,6 +116,7 @@
117	117	'moodbar-restore-intro' => '',
118	118	'moodbar-invalid-item' => 'The system was unable to find the correct feedback item.',
119	119	'moodbar-feedback-action-error' => 'An error occurred when trying to perform this action.',
	120	+ 'moodbar-feedback-response-summary' => '[[$1\|$2]] [[$3\|responded]] this comment $4 ago',
120	121	// Mood types
121	122	'moodbar-type-happy' => '{{GENDER:$1\|Happy}}',
122	123	'moodbar-type-sad' => '{{GENDER:$1\|Sad}}',
—	—	@@ -225,7 +226,7 @@
226	227	'moodbar-header-user' => '{{Identical\|User}}',
227	228	'moodbar-header-comment' => '{{Identical\|Comment}}',
228	229	'moodbar-header-namespace' => '{{Identical\|Namespace}}',
229		~~- 'moodbar-feedback-response-title' => 'The title for appending feedback response text to a user talk page',~~
	230	+ 'moodbar-feedback-response-title' => 'The title for appending feedback response text to a user talk page, $1 is the dashboard page',
230	231	'moodbar-feedback-view-link' => 'link to an individual feedback',
231	232	'moodbar-feedback-filters' => '{{Identical\|Filter}}',
232	233	'moodbar-feedback-filters-type' => '{{Identical\|Mood}}',
—	—	@@ -248,6 +249,7 @@
249	250	'moodbar-hidden-footer-without-log' => '* $1 is a link to restore the item displaying {{msg-mw\|moodbar-feedback-restore}}',
250	251	'moodbar-action-reason' => 'Text for Admin action reason',
251	252	'moodbar-action-reason-required' => 'Text explaining admin action reason is required',
	253	+ 'moodbar-feedback-response-summary' => 'Text providing a summary of a user response, $1 is user page, $2 is user name, $3 is user talk page, $4 is time',
252	254	'moodbar-type-happy' => '$1 is the username that can be used for GENDER',
253	255	'moodbar-type-sad' => '$1 is the username that can be used for GENDER',
254	256	'moodbar-type-confused' => '$1 is the username that can be used for GENDER',
Index: trunk/extensions/MoodBar/ApiFeedbackDashboardResponse.php
—	—	@@ -3,7 +3,7 @@
4	4	class ApiFeedbackDashboardResponse extends ApiBase {
5	5
6	6	public function execute() {
7		~~- global $wgRequest, $wgUser, $wgContLang;~~
	7	+ global $wgRequest, $wgUser, $wgContLang, $wgParser;
8	8
9	9	if ( $wgUser->isAnon() ) {
10	10	$this->dieUsage( "You don't have permission to do that", 'permission-denied' );
—	—	@@ -28,16 +28,17 @@
29	29
30	30	if ( $commenter !== null && $commenter->isAnon() == false ) {
31	31	$talkPage = $commenter->getTalkPage();
	32	+
	33	+ $feedback_link = wfMessage('moodbar-feedback-response-title')->rawParams($wgContLang->getNsText( NS_SPECIAL ) .
	34	+ ':FeedbackDashboard/' . $item->getProperty('feedback'))->escaped();
32	35
33	36	$api = new ApiMain( new FauxRequest( array(
34	37	'action' => 'edit',
35	38	'title' => $talkPage->getFullText(),
36	39	'appendtext' => ( $talkPage->exists() ? "\n\n" : '' ) .
37		~~- '==' . wfMessage('moodbar-feedback-response-title')->escaped() . '==' . "\n\n" .~~
38		~~- '[[' . $wgContLang->getNsText( NS_SPECIAL ) . ':FeedbackDashboard/' . $item->getProperty('feedback') . '\|' .~~
39		~~- wfMessage('moodbar-feedback-view-link')->escaped() . ']]' . "\n\n".~~
40		~~- '[['. $wgUser->getTalkPage()->getFullText() . '\|' . $wgUser->getName() . ']] ' .~~
41		~~- '<nowiki>' . $params['response'] . '</nowiki>',~~
	40	+ $feedback_link . "\n" .
	41	+ '<span id="feedback-dashboard-response-' . $item->getProperty('id') . '"></span>' . "\n\n" .
	42	+ $wgParser->cleanSigInSig($params['response']) . "\n\n~~~~",
42	43	'token' => $params['token'],
43	44	'summary' => '',
44	45	'notminor' => true,
Index: trunk/extensions/MoodBar/ApiQueryMoodBarComments.php
—	—	@@ -45,6 +45,9 @@
46	46	$res = $this->select( __METHOD__ );
47	47	$result = $this->getResult();
48	48	$count = 0;
	49	+
	50	+ $response = SpecialFeedbackDashboard::getResponseSummary( $res );
	51	+
49	52	foreach ( $res as $row ) {
50	53	if ( ++$count > $params['limit'] ) {
51	54	// We've reached the one extra which shows that there are additional rows. Stop here
—	—	@@ -52,7 +55,7 @@
53	56	break;
54	57	}
55	58
56		~~- $vals = $this->extractRowInfo( $row, $prop );~~
	59	+ $vals = $this->extractRowInfo( $row, $prop, $response );
57	60	$fit = $result->addValue( array( 'query', $this->getModuleName() ), null, $vals );
58	61	if ( !$fit ) {
59	62	$this->setContinueEnumParameter( 'continue', $this->getContinue( $row ) );
—	—	@@ -85,7 +88,7 @@
86	89	);
87	90	}
88	91
89		~~- protected function extractRowInfo( $row, $prop ) {~~
	92	+ protected function extractRowInfo( $row, $prop, $response = array() ) {
90	93	global $wgUser;
91	94
92	95	$r = array();
—	—	@@ -114,7 +117,7 @@
115	118	$params[] = 'show-anyway';
116	119	}
117	120
118		~~- $r['formatted'] = SpecialFeedbackDashboard::formatListItem( $row, $params );~~
	121	+ $r['formatted'] = SpecialFeedbackDashboard::formatListItem( $row, $params, $response );
119	122	}
120	123
121	124	if ( $isHidden && !$showHidden ) {

Follow-up revisions

Revision	Commit summary	Author	Date
r104705	followup to r104644 - update the language file	bsitu	17:20, 30 November 2011
r104757	followup to r104644 - change for code review	bsitu	21:47, 30 November 2011

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r104642	added preview mode for responses, allow wikitext, add response icons / langua...	rmoen	01:37, 30 November 2011

Comments

#Comment by Catrope (talk | contribs) 21:10, 30 November 2011

+				if ( $responder =  User::newFromId( $response_detail->mbfr_user_id ) ) {
[...]
+		if ( $feedback = implode( ',', $feedback ) ) {

Don't use assignments in if conditions.

+					$individual_response = wfMsgExt('moodbar-feedback-response-summary', array('parse'),  
+						                         $responder->getUserPage()->getFullText(), 
+						                         htmlspecialchars($responder->getName()), 
+						                         $permalinkTitle . '#feedback-dashboard-response-' . $response_detail->mbfr_id,  
+						                         $responsetime);

You shouldn't be passing an escaped responder name into wfMsgExt in parse mode. The parser will take care of escaping. Also, it would be nice if new code used wfMessage().

+					     array( 'mbfr_mbf_id IN (' . $feedback . ') AND mbfr_user_id != 0' ),

We have a quite rich database API. You should be using something like array( 'mbfr_mbf_id' => $feedback, 'mbfr_user_id != 0' ) where $feedback is an array of IDs.

+					     array( 'ORDER BY' => "mbfr_timestamp DESC, mbfr_id DESC" )

This ordering is not indexed. You should

change this to ORDER BY mbfr_mbf_id DESC, mbfr_timestamp DESC, mbfr_id DESC
add an index on (mbfr_mbf_id, mbfr_timestamp, mbfr_id). You can extend the existing index for this

+			$feedback_link = wfMessage('moodbar-feedback-response-title')->rawParams($wgContLang->getNsText( NS_SPECIAL ) . 
+				         ':FeedbackDashboard/' . $item->getProperty('feedback'))->escaped();

You should use params() here instead of rawParams(). The arguments to rawParams() are potentially interpreted as HTML (because they bypass escaping), and what you're passing in is not HTML so it should be escaped.

Status & tagging log

21:51, 30 November 2011 Catrope (talk | contribs) changed the status of r104644 [removed: new added: resolved]
21:49, 30 November 2011 Bsitu (talk | contribs) changed the status of r104644 [removed: fixme added: new]
21:10, 30 November 2011 Catrope (talk | contribs) changed the status of r104644 [removed: new added: fixme]