r103881 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r103880‎ | r103881 | r103882 >
Date:01:45, 22 November 2011
Author:bsitu
Status:ok (Comments)
Tags:
Comment:
MoodBar Phase 2 - Feedback response API
Modified paths:
  • /trunk/extensions/MoodBar/ApiFeedbackDashboardResponse.php (added) (history)
  • /trunk/extensions/MoodBar/FeedbackResponseItem.php (added) (history)
  • /trunk/extensions/MoodBar/MoodBar.hooks.php (modified) (history)
  • /trunk/extensions/MoodBar/MoodBar.i18n.php (modified) (history)
  • /trunk/extensions/MoodBar/MoodBar.php (modified) (history)
  • /trunk/extensions/MoodBar/sql/moodbar_feedback_response.sql (added) (history)

Diff [purge]

Index: trunk/extensions/MoodBar/MoodBar.i18n.php
——@@ -75,6 +75,8 @@
7676 'moodbar-header-own-talk' => 'Own talk page',
7777 // Special:MoodBarFeedback
7878 'moodbar-feedback-title' => 'Feedback dashboard',
 79+ 'moodbar-feedback-response-title' => 'Feedback dashboard response',
 80+ 'moodbar-feedback-view-link' => '(View the feedback)',
7981 'moodbar-feedback-filters' => 'Filters',
8082 'moodbar-feedback-filters-type' => 'Mood:',
8183 'moodbar-feedback-filters-type-happy' => 'Happy',
Index: trunk/extensions/MoodBar/ApiFeedbackDashboardResponse.php
——@@ -0,0 +1,116 @@
 2+<?php
 3+
 4+class ApiFeedbackDashboardResponse extends ApiBase {
 5+
 6+ public function execute() {
 7+ global $wgRequest, $wgUser, $wgContLang;
 8+
 9+ if ( $wgUser->isAnon() ) {
 10+ $this->dieUsage( "You don't have permission to do that", 'permission-denied' );
 11+ }
 12+
 13+ $params = $this->extractRequestParams();
 14+
 15+ //Response Object
 16+ $item = MBFeedbackResponseItem::create( array() );
 17+
 18+ $setParams = array();
 19+ foreach( $params as $key => $value ) {
 20+ if ( $item->isValidKey( $key ) ) {
 21+ $setParams[$key] = $value;
 22+ }
 23+ }
 24+
 25+ $item->setProperties( $setParams );
 26+ $item->save();
 27+
 28+ $commenter = $item->getProperty('feedbackitem')->getProperty('user');
 29+
 30+ if ( $commenter !== null && $commenter->isAnon() == false ) {
 31+ $talkPage = $commenter->getTalkPage();
 32+
 33+ $api = new ApiMain( new FauxRequest( array(
 34+ 'action' => 'edit',
 35+ 'title' => $talkPage->getFullText(),
 36+ '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>',
 42+ 'token' => $params['token'],
 43+ 'summary' => '',
 44+ 'notminor' => true,
 45+ ), true, array( 'wsEditToken' => $wgRequest->getSessionData( 'wsEditToken' ) ) ), true );
 46+
 47+ $api->execute();
 48+ }
 49+
 50+ $result = array( 'result' => 'success' );
 51+ $this->getResult()->addValue( null, $this->getModuleName(), $result );
 52+ }
 53+
 54+ public function needsToken() {
 55+ return true;
 56+ }
 57+
 58+ public function getTokenSalt() {
 59+ return '';
 60+ }
 61+
 62+ public function getAllowedParams() {
 63+ return array(
 64+ 'feedback' => array(
 65+ ApiBase::PARAM_REQUIRED => true,
 66+ ApiBase::PARAM_TYPE => 'integer'
 67+ ),
 68+ 'response' => array(
 69+ ApiBase::PARAM_REQUIRED => true,
 70+ ),
 71+ 'anonymize' => array(
 72+ ApiBase::PARAM_TYPE => 'boolean',
 73+ ),
 74+ 'editmode' => array(
 75+ ApiBase::PARAM_TYPE => 'boolean',
 76+ ),
 77+ 'useragent' => null,
 78+ 'system' => null,
 79+ 'locale' => null,
 80+ 'bucket' => null,
 81+ 'token' => null,
 82+ );
 83+ }
 84+
 85+ public function mustBePosted() {
 86+ return true;
 87+ }
 88+
 89+ public function isWriteMode() {
 90+ return true;
 91+ }
 92+
 93+ public function getVersion() {
 94+ return __CLASS__ . ': $Id: ApiMoodBar.php 93113 2011-07-25 21:03:33Z reedy $';
 95+ }
 96+
 97+ public function getParamDescription() {
 98+ return array(
 99+ 'feedback' => 'The moodbar feedback unique identifier',
 100+ 'response' => 'The feedback text',
 101+ 'anonymize' => 'Whether to hide user information',
 102+ 'editmode' => 'Whether or not the feedback context is in edit mode',
 103+ 'bucket' => 'The testing bucket, if any',
 104+ 'useragent' => 'The User-Agent header of the browser',
 105+ 'system' => 'The operating system being used',
 106+ 'locale' => 'The locale in use',
 107+ 'token' => 'An edit token',
 108+ );
 109+ }
 110+
 111+ public function getDescription() {
 112+ return 'Allows users to submit response to a feedback about their experiences on the site';
 113+ }
 114+
 115+}
 116+
 117+?>
Property changes on: trunk/extensions/MoodBar/ApiFeedbackDashboardResponse.php
___________________________________________________________________
Added: svn:eol-style
1118 + native
Index: trunk/extensions/MoodBar/MoodBar.hooks.php
——@@ -101,6 +101,9 @@
102102
103103 $updater->addExtensionUpdate( array( 'addField', 'moodbar_feedback',
104104 'mbf_hidden_state', dirname(__FILE__).'/sql/mbf_hidden_state.sql', true ) );
 105+
 106+ $updater->addExtensionUpdate( array( 'addTable', 'moodbar_feedback_response',
 107+ dirname(__FILE__).'/sql/moodbar_feedback_response.sql', true ) );
105108
106109 return true;
107110 }
Index: trunk/extensions/MoodBar/sql/moodbar_feedback_response.sql
——@@ -0,0 +1,26 @@
 2+CREATE TABLE /*_*/moodbar_feedback_response (
 3+ mbfr_id int unsigned NOT NULL PRIMARY KEY auto_increment, -- Primary key
 4+
 5+ mbfr_mbf_id int unsigned NOT NULL, -- Primary key of moodbar_feedback table
 6+
 7+ -- User who provided the response
 8+ mbfr_user_id int unsigned NOT NULL, -- User ID, or zero
 9+ mbfr_user_ip varchar(255) binary NULL, -- If anonymous, user's IP address
 10+
 11+ mbfr_commenter_editcount int unsigned NOT NULL, -- number of edit for the user who writes the feedback
 12+ mbfr_user_editcount int unsigned NOT NULL, -- number of edit for the responder
 13+
 14+ -- The response itself
 15+ mbfr_response_text text NOT NULL,
 16+
 17+ -- Options and context
 18+ mbfr_timestamp varchar(14) binary NOT NULL, -- When response was received
 19+ mbfr_anonymous tinyint unsigned NOT NULL DEFAULT 0, -- Anonymity
 20+ mbfr_system_type varchar(64) binary NULL, -- Operating System
 21+ mbfr_user_agent varchar(255) binary NULL, -- User-Agent header
 22+ mbfr_locale varchar(32) binary NULL, -- The locale of the user's browser
 23+ mbfr_editing tinyint unsigned NOT NULL, -- Whether or not the user was editing
 24+ mbfr_bucket varchar(128) binary NULL -- Bucket, for A/B testing
 25+) /*$wgDBTableOptions*/;
 26+
 27+CREATE INDEX /*i*/mbfr_mbf_id ON /*_*/moodbar_feedback_response (mbfr_mbf_id);
Property changes on: trunk/extensions/MoodBar/sql/moodbar_feedback_response.sql
___________________________________________________________________
Added: svn:eol-style
128 + native
Index: trunk/extensions/MoodBar/FeedbackResponseItem.php
——@@ -0,0 +1,285 @@
 2+<?php
 3+
 4+/**
 5+ * This class represents a single piece of MoodBar feedback response
 6+ * @Todo: Make a base class for both MBFeedbackItem and MBFeedbackResponseItem
 7+ * so common functions can be re-used
 8+ */
 9+class MBFeedbackResponseItem {
 10+ /** Container for the data **/
 11+ protected $data;
 12+ /** Valid data keys **/
 13+ protected $validMembers = array(
 14+ // Feedback response essentials
 15+ 'id', // ID in the database for response
 16+ 'feedback', // ID in the database for moodbar_feedback
 17+ 'feedbackitem', // the feedback that the user responds to
 18+ 'user', // user object
 19+ 'anonymize',
 20+ 'commenter-editcount', // Number of edit for the user writes the feedback
 21+ 'user-editcount', // Number of edit for the responder
 22+ 'response', // The response itself
 23+ 'timestamp', // When response was received
 24+ 'system', // Operating System
 25+ 'useragent' , // User-Agent header
 26+ 'locale', // The locale of the user's browser
 27+ 'editmode', // Whether or not the user was editing
 28+ 'bucket' // Bucket, for A/B testing
 29+ );
 30+ /**
 31+ * Default constructor.
 32+ * Don't use this, use MBFeedbackResponseItem::create
 33+ */
 34+ protected function __construct() {
 35+ $this->data = array_fill_keys( $this->validMembers, null );
 36+
 37+ // Non-nullable boolean fields
 38+ $this->setProperty('anonymize', false);
 39+ $this->setProperty('editmode', false);
 40+ }
 41+
 42+ /**
 43+ * Factory function to create a new MBFeedbackResponseItem
 44+ * @param $info Associative array of values
 45+ * Valid keys: feedbackitem, user, response-text, timestamp,
 46+ * useragent, system, locale, bucket
 47+ * @return MBFeedbackItem object.
 48+ */
 49+ public static function create( $info ) {
 50+ $newObject = new self();
 51+ $newObject->initialiseNew( $info );
 52+ return $newObject;
 53+ }
 54+
 55+ /**
 56+ * Initialiser for new MBFeedbackResponseItem
 57+ * @param $info Associative array of values
 58+ * @see MBFeedbackItem::create
 59+ */
 60+ protected function initialiseNew( $info ) {
 61+ global $wgUser;
 62+
 63+ $template = array(
 64+ 'user' => $wgUser,
 65+ 'timestamp' => wfTimestampNow(),
 66+ );
 67+
 68+ $this->setProperties( $template );
 69+ $this->setProperties( $info );
 70+ }
 71+
 72+ /**
 73+ * Factory function to load an MBFeedbackResponseItem from a DB row.
 74+ * @param $row A row, from DatabaseBase::fetchObject
 75+ * @return MBFeedResponseback object.
 76+ */
 77+ public static function load( $row ) {
 78+ $newObject = new self();
 79+ $newObject->initialiseFromRow( $row );
 80+ return $newObject;
 81+ }
 82+
 83+ /**
 84+ * Initialiser for MBFeedbackResponseItems loaded from the database
 85+ * @param $row A row object from DatabaseBase::fetchObject
 86+ * @see MBFeedbackItem::load
 87+ */
 88+ protected function initialiseFromRow( $row ) {
 89+ static $propMappings = array(
 90+ 'id' => 'mbfr_id',
 91+ 'feedback' => 'mbfr_mbf_id',
 92+ 'response' => 'mbfr_response_text',
 93+ 'timestamp' => 'mbfr_timestamp',
 94+ 'anonymize' => 'mbfr_anonymous',
 95+ 'useragent' => 'mbfr_user_agent',
 96+ 'system' => 'mbfr_system_type',
 97+ 'locale' => 'mbfr_locale',
 98+ 'bucket' => 'mbfr_bucket',
 99+ 'editmode' => 'mbfr_editing',
 100+ 'commenter-editcount',
 101+ 'user-editcount'
 102+ );
 103+
 104+ $properties = array();
 105+
 106+ foreach( $propMappings as $property => $field ) {
 107+ if ( isset( $row->$field ) ) {
 108+ $properties[$property] = $row->$field;
 109+ }
 110+ }
 111+
 112+ if ( $row->mbfr_user_id > 0 ) {
 113+ $properties['user'] = User::newFromId( $row->mbfr_user_id );
 114+ } elseif ( $row->mbfr_user_ip ) {
 115+ $properties['user'] = User::newFromName( $row->mbfr_user_ip );
 116+ }
 117+ else {
 118+ //Error
 119+ $properties['user'] = '';
 120+ }
 121+
 122+ $this->setProperties( $properties );
 123+
 124+ }
 125+
 126+ /**
 127+ * Set a group of properties. Throws an exception on invalid property.
 128+ * @param $values An associative array of properties to set.
 129+ */
 130+ public function setProperties( $values ) {
 131+
 132+ foreach( $values as $key => $value ) {
 133+ if ( ! $this->isValidKey($key) ) {
 134+ throw new MWException( "Attempt to set invalid property $key" );
 135+ }
 136+
 137+ if ( ! $this->validatePropertyValue($key, $value) ) {
 138+ throw new MWException( "Attempt to set invalid value for $key" );
 139+ }
 140+
 141+ $this->data[$key] = $value;
 142+ }
 143+ }
 144+
 145+ /**
 146+ * Set a group of values.
 147+ * @param $key The property to set.
 148+ * @param $value The value to give it.
 149+ */
 150+ public function setProperty( $key, $value ) {
 151+ $this->setProperties( array( $key => $value ) );
 152+ }
 153+
 154+ /**
 155+ * Get a property.
 156+ * @param $key The property to get
 157+ * @return The property value.
 158+ */
 159+ public function getProperty( $key ) {
 160+ if ( ! $this->isValidKey($key) ) {
 161+ throw new MWException( "Attempt to get invalid property $key" );
 162+ }
 163+
 164+ return $this->data[$key];
 165+ }
 166+
 167+ /**
 168+ * Check a property key for validity.
 169+ * If a property key is valid, it will be prefilled to NULL.
 170+ * @param $key The property key to check.
 171+ */
 172+ public function isValidKey( $key ) {
 173+ return in_array( $key, $this->validMembers );
 174+ }
 175+
 176+ /**
 177+ * Check if a property value is valid for that property
 178+ * @param $key The key of the property to check.
 179+ * @param $value The value to check
 180+ * @return Boolean
 181+ */
 182+ public function validatePropertyValue( $key, $value ) {
 183+ if ( $key == 'user' ) {
 184+ return $value instanceof User;
 185+ } elseif ( $key == 'feedback' ) {
 186+ if( $value <= 0 ) {
 187+ return false;
 188+ }
 189+ if( !$this->setFeedbackItem($value) ) {
 190+ return false;
 191+ }
 192+ return true;
 193+ } elseif ( $key == 'feedbackitem' ) {
 194+ return $value instanceof MBFeedbackItem;
 195+ } elseif ( $key == 'response' ) {
 196+ $comment_len = mb_strlen( $value );
 197+ return $comment_len > 0 && $comment_len <= 5000;
 198+ }
 199+
 200+ return true;
 201+ }
 202+
 203+ /**
 204+ * Writes this MBFeedbackItem to the database.
 205+ * Throws an exception if this it is already in the database.
 206+ * @return The MBFeedbackItem's new ID.
 207+ */
 208+ public function save() {
 209+
 210+ $user = $this->getProperty('user');
 211+
 212+ // Add edit count if necessary
 213+ if ( $this->getProperty('user-editcount') === null && $user !== null )
 214+ {
 215+ $this->setProperty( 'user-editcount', $user->isAnon() ? 0 : $user->getEditCount() );
 216+
 217+ }
 218+
 219+ if($this->getProperty('commenter-editcount') === null) {
 220+ $commenter = $this->getProperty('feedbackitem')->getProperty('user');
 221+ $this->setProperty( 'commenter-editcount', $commenter->isAnon() ? 0 : $commenter->getEditCount() );
 222+ }
 223+
 224+ $dbw = wfGetDB( DB_MASTER );
 225+
 226+ $row = array(
 227+ 'mbfr_mbf_id' => $this->getProperty('feedback'),
 228+ 'mbfr_commenter_editcount' => $this->getProperty('commenter-editcount'),
 229+ 'mbfr_user_editcount' => $this->getProperty('user-editcount'),
 230+ 'mbfr_response_text' => $this->getProperty('response'),
 231+ 'mbfr_timestamp' => $dbw->timestamp($this->getProperty('timestamp')),
 232+ 'mbfr_system_type' => $this->getProperty('system'),
 233+ 'mbfr_user_agent' => $this->getProperty('useragent'),
 234+ 'mbfr_locale' => $this->getProperty('locale'),
 235+ 'mbfr_bucket' => $this->getProperty('bucket'),
 236+ 'mbfr_editing' => $this->getProperty('editmode')
 237+ );
 238+
 239+ if($user->isAnon()) {
 240+ $row['mbfr_user_id'] = 0;
 241+ $row['mbfr_user_ip'] = $user->getName();
 242+ }
 243+ else {
 244+ $row['mbfr_user_id'] = $user->getId();
 245+ }
 246+
 247+ if ( $this->getProperty('id') ) {
 248+ $row['mbfr_id'] = $this->getProperty('id');
 249+ $dbw->replace( 'moodbar_feedback_response', array('mbfr_id'), $row, __METHOD__ );
 250+ } else {
 251+ $row['mbfr_id'] = $dbw->nextSequenceValue( 'moodbar_feedback_response_mbfr_id' );
 252+ $dbw->insert( 'moodbar_feedback_response', $row, __METHOD__ );
 253+ $this->setProperty( 'id', $dbw->insertId() );
 254+ }
 255+
 256+ return $this->getProperty('id');
 257+ }
 258+
 259+ /**
 260+ * Gets the valid types of a feedback item.
 261+ */
 262+ public static function getValidTypes() {
 263+ return self::$validTypes;
 264+ }
 265+
 266+ /**
 267+ * Get the Feedback Item this Response is associated to
 268+ * @param $mbf_id mbfr_mbf_id in moodbar_feedback_response table
 269+ * @param MBFeedbackItem Object/bool
 270+ */
 271+ public function setFeedbackItem($mbf_id) {
 272+ $dbr = wfGetDB( DB_SLAVE );
 273+
 274+ $row = $dbr->selectRow( 'moodbar_feedback', '*',
 275+ array( 'mbf_id' => $mbf_id ), __METHOD__ );
 276+
 277+ if( $row !== false ) {
 278+ $this->setProperties( array ( 'feedbackitem' => MBFeedbackItem::load( $row ) ) );
 279+ return true;
 280+ }
 281+ else {
 282+ return false;
 283+ }
 284+ }
 285+
 286+}
Property changes on: trunk/extensions/MoodBar/FeedbackResponseItem.php
___________________________________________________________________
Added: svn:eol-style
1287 + native
Index: trunk/extensions/MoodBar/MoodBar.php
——@@ -15,6 +15,7 @@
1616
1717 // Object model
1818 $wgAutoloadClasses['MBFeedbackItem'] = dirname(__FILE__).'/FeedbackItem.php';
 19+$wgAutoloadClasses['MBFeedbackResponseItem'] = dirname(__FILE__).'/FeedbackResponseItem.php';
1920 $wgAutoloadClasses['MoodBarFormatter'] = dirname(__FILE__).'/Formatter.php';
2021
2122 // API
——@@ -24,6 +25,8 @@
2526 $wgAPIListModules['moodbarcomments'] = 'ApiQueryMoodBarComments';
2627 $wgAutoloadClasses['ApiFeedbackDashboard'] = dirname(__FILE__).'/ApiFeedbackDashboard.php';
2728 $wgAPIModules['feedbackdashboard'] = 'ApiFeedbackDashboard';
 29+$wgAutoloadClasses['ApiFeedbackDashboardResponse'] = dirname(__FILE__).'/ApiFeedbackDashboardResponse.php';
 30+$wgAPIModules['feedbackdashboardresponse'] = 'ApiFeedbackDashboardResponse';
2831
2932 // Hooks
3033 $wgAutoloadClasses['MoodBarHooks'] = dirname(__FILE__).'/MoodBar.hooks.php';

Follow-up revisions

RevisionCommit summaryAuthorDate
r103883followup to r103881 - change the function return date type in the commentbsitu02:03, 22 November 2011
r103939followup to r103881 - add message documentation and change function visibilit...bsitu18:55, 22 November 2011
r103964followup to r103881, remove the unnecessary field bucket since it is only use...bsitu21:57, 22 November 2011
r104055followup to r103881 - change datatype of user_ip to varbinary(40) and replace...bsitu17:39, 23 November 2011

Comments

#Comment by Johnduhart (talk | contribs)   11:02, 22 November 2011

{{subst:Messagedocumentation}}

#Comment by Johnduhart (talk | contribs)   11:03, 22 November 2011

Er, Please add message documentation for the newly added messages. Thanks.

#Comment by Bsitu (talk | contribs)   20:51, 22 November 2011

Fixed in /trunk r103939

#Comment by Raindrift (talk | contribs)   02:54, 23 November 2011 
+	mbfr_user_ip varchar(255) binary NULL, -- If anonymous, user's IP address

Elsewhere in Mediawiki, IP addresses are stored as varbinary(40). You may want to make that change, since it shouldn't effect the rest of your code and it'd be consistent. Also, performance might be slightly higher.

I'm not really clear on why MBFeedbackResponseItem has a constructor (which the comment says not to use, but which does things anyway), a separate create() method that's not called from the constructor, and then two initializers. The two separate init schemes makes sense, but why not choose which one to run directly from the constructor? 

throw new MWException( "Attempt to set invalid property $key" );

If you create new exceptions for things like this, and add them to @throws in your docblock, it'll be clearer what's going on later, and it'll be possible for a caller to catch some exceptions and not others. At the end of the file, you can just create new exception subclasses like so: 

class MWFeedbackResponseItemPropertyException extends MWException {};

or name it something shorter... the point is to tell it apart from every other MWException. if you want to see an example, check out includes/upload/UploadStash.php. A lot of Mediawiki code doesn't do this, so I guess it's optional, but more granularity here is better.

I'm not sure if any of these are FIXME-worthy. I'll set this rev back to new so maybe other reviewers will take a look at it.

#Comment by Bsitu (talk | contribs)   05:09, 23 November 2011

For the constructor, I tried to make MBFeedbackResponseItem have the same coding style as MBFeedbackItem. I think here is how it works, create() and load() are used to create new item and existing db item. Each of these methods takes different argument, the first one is taking an array and the second one is taking a DbObject row. Having the two method to create object makes the code more informative and the constructor doesn't have to take a random data type: array or DbObject row. This is just my thought about it.

#Comment by Raindrift (talk | contribs)   19:35, 23 November 2011

That makes sense, if for no other reason than consistency with the other class. I can see the advantages to doing it this way, though I'm uncertain whether I'd do the same thing. Thanks for explaining!

Status & tagging log