r109216 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109215‎ | r109216 | r109217 >
Date:21:26, 17 January 2012
Author:bsitu
Status:ok (Comments)
Tags:
Comment:
Surfacing MarkAsHelpful on FeedbackDashboard page
Modified paths:
  • /trunk/extensions/MoodBar/MoodBar.i18n.php (modified) (history)
  • /trunk/extensions/MoodBar/SpecialFeedbackDashboard.php (modified) (history)
  • /trunk/extensions/MoodBar/include/MoodBarUtil.php (modified) (history)

Diff [purge]

Index: trunk/extensions/MoodBar/include/MoodBarUtil.php
——@@ -137,4 +137,14 @@
138138
139139 }
140140
 141+ /**
 142+ * Check if MarkAsHelpful extension is enabled
 143+ * @return bool
 144+ */
 145+ public static function isMarkAsHelpfulEnabled() {
 146+ global $wgMarkAsHelpfulType;
 147+
 148+ return is_array( $wgMarkAsHelpfulType ) && in_array( 'mbresponse', $wgMarkAsHelpfulType );
 149+ }
 150+
141151 }
\ No newline at end of file
Index: trunk/extensions/MoodBar/SpecialFeedbackDashboard.php
——@@ -319,15 +319,25 @@
320320 $responder = User::newFromRow( $response_detail );
321321
322322 if ( $responder && !$responder->isAnon() ) {
323 - $responsetime = MoodBarUtil::formatTimeSince( wfTimestamp( TS_UNIX, $response_detail->mbfr_timestamp ) );
 323+ $responsetime = MoodBarUtil::formatTimeSince( wfTimestamp( TS_UNIX, $response_detail->mbfr_timestamp ) );
 324+ $commenter = $feedbackItem->getProperty('user');
 325+ $permalinkTitle = $commenter->getTalkPage()->getFullText();
324326
325 - $permalinkTitle = $feedbackItem->getProperty('user')->getTalkPage()->getFullText();
326 -
327 - $individual_response = wfMsgExt('moodbar-feedback-response-summary', array('parse'),
328 - $responder->getUserPage()->getFullText(),
329 - $responder->getName(),
330 - $permalinkTitle . '#feedback-dashboard-response-' . $response_detail->mbfr_id,
331 - $responsetime);
 327+ if ( property_exists( $response_detail, 'mah_id' ) && intval($response_detail->mah_id ) > 0 ) {
 328+ $individual_response = wfMsgExt('moodbar-feedback-response-helpful-summary', array('parse'),
 329+ $responder->getUserPage()->getFullText(),
 330+ $responder->getName(),
 331+ $permalinkTitle . '#feedback-dashboard-response-' . $response_detail->mbfr_id,
 332+ $responsetime,
 333+ $commenter->getUserPage()->getFullText(),
 334+ $commenter->getName());
 335+ } else {
 336+ $individual_response = wfMsgExt('moodbar-feedback-response-summary', array('parse'),
 337+ $responder->getUserPage()->getFullText(),
 338+ $responder->getName(),
 339+ $permalinkTitle . '#feedback-dashboard-response-' . $response_detail->mbfr_id,
 340+ $responsetime);
 341+ }
332342 $showResponseBox = false;
333343
334344 $responseElements = <<<HTML
——@@ -714,13 +724,13 @@
715725 }
716726
717727 /**
718 - * Get the latest response summary for a set of feedback,
 728+ * Get the latest response summary for a set of feedback
719729 * @param $res Iterator of Db row with index mbf_latest_response for feedback
720730 * @return array
721731 */
722732 public static function getResponseSummary( $res ) {
723733 $dbr = wfGetDB( DB_SLAVE );
724 -
 734+
725735 $mbfrIds = array();
726736
727737 foreach ( $res as $row ) {
——@@ -732,12 +742,24 @@
733743 $response = array();
734744
735745 if ( count( $mbfrIds ) > 0 ) {
736 - $res = $dbr->select( array( 'moodbar_feedback_response', 'user' ),
737 - array( 'mbfr_id', 'mbfr_mbf_id', 'mbfr_timestamp', 'user_id', 'user_name', 'user_real_name' ),
738 - array( 'mbfr_id' => $mbfrIds, 'mbfr_user_id = user_id' ),
739 - __METHOD__
740 - );
741746
 747+ $table = array( 'user', 'moodbar_feedback_response' );
 748+ $select = array( 'mbfr_id', 'mbfr_mbf_id', 'mbfr_timestamp', 'user_id', 'user_name', 'user_real_name' );
 749+ $conds = array( 'mbfr_id' => $mbfrIds, 'mbfr_user_id = user_id' );
 750+ $tableJoin = array();
 751+
 752+ // Adding markashelpful data if the extension is enabled
 753+ if ( MoodBarUtil::isMarkAsHelpfulEnabled() ) {
 754+ $table[] = 'moodbar_feedback';
 755+ $table[] = 'mark_as_helpful';
 756+ // Is there a workaround that does not specify INNER JOIN explicitly?
 757+ $tableJoin['moodbar_feedback'] = array( 'INNER JOIN', 'mbfr_mbf_id = mbf_id' );
 758+ $tableJoin['mark_as_helpful'] = array( 'LEFT JOIN', "mah_type = 'mbresponse' AND mah_item = mbfr_id AND mah_user_id = mbf_user_id" );
 759+ $select[] = 'mah_id';
 760+ }
 761+
 762+ $res = $dbr->select( $table, $select, $conds, __METHOD__, array(), $tableJoin );
 763+
742764 foreach ( $res as $row ) {
743765 $response[$row->mbfr_mbf_id] = $row;
744766 }
——@@ -745,5 +767,5 @@
746768
747769 return $response;
748770 }
749 -
 771+
750772 }
\ No newline at end of file
Index: trunk/extensions/MoodBar/MoodBar.i18n.php
——@@ -140,6 +140,7 @@
141141 'moodbar-invalid-item' => 'The system was unable to find the correct feedback item.',
142142 'moodbar-feedback-action-error' => 'An error occurred when trying to perform this action.',
143143 'moodbar-feedback-response-summary' => '[[$1|$2]] [[$3|responded]] $4 ago',
 144+ 'moodbar-feedback-response-helpful-summary' => '[[$1|$2]] [[$3|responded]] $4 ago and [[$5|$6]] thinks it helpful',
144145 'moodbar-feedback-edit-summary' => 'Response to [[Special:FeedbackDashboard/$1|user feedback]]: $2',
145146 'moodbar-feedback-top-responders-title' => 'Top Responders',
146147 // Mood types
——@@ -210,12 +211,10 @@
211212 This is a feature in development. See [[mw:MoodBar 0.1/Design]] for background information.',
212213 'moodbar-trigger-feedback' => 'Link text of the MoodBar overlay trigger. $1 is the SITENAME.',
213214 'moodbar-trigger-editing' => "Link text of the MoodBar overlay trigger. \$1 is the SITENAME. The implied sentence is ''\"Using [Sitename] made me happy/sad/...\"''. See [[mw:MoodBar 0.1/Design]] for background development information.",
214 - 'moodbar-weeks' => 'Full word for "weeks". $1 is the number of weeks. Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}}',
215 - 'moodbar-months' => 'Full word for "months". $1 is the number of months. Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}}.
216 -{{Identical|Month}}',
217 - 'moodbar-years' => 'Full word for "years". $1 is the number of years. Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}}.
218 -{{Identical|Year}}',
219 - 'moodbar-seconds' => 'The phrase "less than 1 minute", Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}}',
 215+ 'moodbar-weeks' => 'Full word for "weeks". $1 is the number of weeks. Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}} and {{msg-moodbar|moodbar-feedback-response-helpful-summary}}',
 216+ 'moodbar-months' => 'Full word for "months". $1 is the number of months. Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}} and {{msg-moodbar|moodbar-feedback-response-helpful-summary}}',
 217+ 'moodbar-years' => 'Full word for "years". $1 is the number of years. Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}} and {{msg-moodbar|moodbar-feedback-response-helpful-summary}}',
 218+ 'moodbar-seconds' => 'The phrase "less than 1 minute", Part of variable $1 in {{msg-mw|Ago}} and variable $4 in {{msg-moodbar|moodbar-feedback-response-summary}} and {{msg-moodbar|moodbar-feedback-response-helpful-summary}}',
220219 'moodbar-close' => 'Link text of the close-button. Make sure to include parentheses.
221220
222221 See also:
——@@ -315,7 +314,9 @@
316315 'moodbar-hidden-footer-without-log' => '* $1 is a link to restore the item displaying {{msg-mw|moodbar-feedback-restore}}',
317316 'moodbar-action-reason' => 'Text for Admin action reason',
318317 'moodbar-action-reason-required' => 'Text explaining admin action reason is required',
319 - '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',
 318+ 'moodbar-feedback-response-summary' => 'Text providing a summary of a user response, $1 is responder user page, $2 is responder user name, $3 is commenter talk page, $4 is time',
 319+ 'moodbar-feedback-response-helpful-summary' => 'Text providing a summary of a user response and indicating that commenter has marked the response as helpful, $1 is responder user page, $2 is responder user name,
 320+$3 is commenter user talk page, $4 is time, $5 is commenter user page, $6 is commenter user name',
320321 'moodbar-feedback-edit-summary' => 'Auto generated Edit summary for feedback response, $1 is the feedback id and $2 is the response text',
321322 'moodbar-feedback-top-responders-title' => 'The title for Top Responders, which is located below feedback dashboard filter',
322323 'moodbar-type-happy' => '$1 is the username that can be used for GENDER. Message is used on Special:FeedbackDashboard.',

Comments

#Comment by Raindrift (talk | contribs)   23:42, 23 January 2012

To answer the question in your comment: since INNER JOIN is the default, you could probably put 'mbfr_mbf_id = mbf_id' in $conds, add 'moodbar_feedback' to $table, and remove $tableJoin['moodbar_feedback'] altogether. That is, place the join condition in the where clause like with the other two tables. That is, unless you're specifically relying on INNER JOIN's higher precedence to make the LEFT JOIN work against the correct table, but it doesn't seem that way... the ON clause should handle it (does this even make sense?)

Aside from that, this looks good, except that I think you may want to poke at the indices a little. I'll go one table at a time:

moodbar_feedback: queries against (mbf_user_id, mbf_id), but I don't think there's a concatenated index that begins with those two columns.

moodbar_feedback_response: queries against (mbfr_id, mbfr_user_id, mbfr_mbf_id), same as above, I can't find an index that covers all of them.

mark_as_helpful: queries against (mah_type, mah_item, mah_user_id), looks good, 'mah_type_item_user_id' covers it.

user: just user_id, which is indexed, of course.  :)

Some of these tables have a lot of indices already, and I bet you could extend an existing index to do what you want rather than adding a new one. For example, the 'mbfr_mbf_mbfr_id' index could just have 'mbfr_user_id' tacked on the end.

It's also possible that this doesn't matter because the intersection of non-indexed rows will be small. I haven't actually run an EXPLAIN on the query with a real dataset...

#Comment by Bsitu (talk | contribs)   00:26, 24 January 2012

hmm, I am not sure if we need a composite index for (mbf_id, mbf_user_id) and (mbfr_id, mbfr_user_id, mbfr_mbf_id). This query is accessed by a constant list of mbfr_id, both mbf_id and mbfr_id are primary keys and mbfr_id to mbf_id is a one-to-one relation, I think these two keys ensure that the query scans at most the number of mbfr_ids from the function parameter

#Comment by Raindrift (talk | contribs)   00:31, 24 January 2012

You are totally right! The mbfr_id IN (stuff...) is going to constrain it to something reasonable, at least assuming that list is smaller than a few hundred elements. Sounds good.

#Comment by Bsitu (talk | contribs)   00:45, 24 January 2012

Yeap, the list has about 20 elements

Status & tagging log