r108187 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r108186‎ \| r108187 \| r108188 >
Date:	00:05, 6 January 2012
Author:	bsitu
Status:	resolved (Comments)
Tags:
Comment:	Adding "Show unanswered" filter to feedback dashboard
Modified paths:	/trunk/extensions/MoodBar/ApiQueryMoodBarComments.php (modified) (history) /trunk/extensions/MoodBar/MoodBar.i18n.php (modified) (history) /trunk/extensions/MoodBar/SpecialFeedbackDashboard.php (modified) (history) /trunk/extensions/MoodBar/modules/ext.moodBar.dashboard/ext.moodBar.dashboard.js (modified) (history)

Diff [purge]

Index: trunk/extensions/MoodBar/SpecialFeedbackDashboard.php
—	—	@@ -49,8 +49,13 @@
50	50	if ( count( $filters ) ) {
51	51	$filterType = 'filtered';
52	52	}
53		-
54		~~- $filters['myresponse'] = $wgRequest->getVal( 'myresponse' );~~
	53	+
	54	+ // UI should allow users to select one or none
	55	+ if ( $wgRequest->getVal( 'myresponse' ) == '1' ) {
	56	+ $filters['responsefilter'] = 'myresponse';
	57	+ } elseif ( $wgRequest->getVal( 'showunanswered' ) == '1' ) {
	58	+ $filters['responsefilter'] = 'showunanswered';
	59	+ }
55	60	}
56	61	// Do the query
57	62	$backwards = $wgRequest->getVal( 'dir' ) === 'prev';
—	—	@@ -141,11 +146,19 @@
142	147	if ( !$wgUser->isAnon() ) {
143	148	$myResponseMsg = wfMessage( 'moodbar-feedback-filters-my-response' )->escaped();
144	149	$myResponseCheckbox = Xml::check( 'myresponse', $wgRequest->getCheck( 'myresponse' ),
145		~~- array( 'id' => 'fbd-filters-my-response', 'value' => '1' ) );~~
	150	+ array( 'id' => 'fbd-filters-my-response', 'value' => '1' ) );
146	151
147	152	$myResponseFilter = '<label for="fbd-filters-my-response" id="fbd-filters-type-my-response-label" class="fbd-filters-label">' .
148		~~- $myResponseCheckbox . $myResponseMsg . '</label>';~~
	153	+ $myResponseCheckbox . $myResponseMsg . '</label>';
149	154	}
	155	+
	156	+ // Show unanswered filter
	157	+ $showUnansweredMsg = wfMessage( 'moodbar-feedback-filters-show-unanswered' )->escaped();
	158	+ $showUnansweredCheckbox = Xml::check( 'showunanswered', $wgRequest->getCheck( 'showunanswered' ),
	159	+ array( 'id' => 'fbd-filters-show-unanswered', 'value' => '1' ) );
	160	+
	161	+ $showUnansweredFilter = '<label for="fbd-filters-show-unanswered" id="fbd-filters-type-show-unanswered-label" class="fbd-filters-label">' .
	162	+ $showUnansweredCheckbox . $showUnansweredMsg . '</label>';
150	163
151	164	return <<<HTML
152	165	<div id="fbd-description">
—	—	@@ -179,6 +192,7 @@
180	193	<label for="fbd-filters-username" class="fbd-filters-label">$usernameMsg</label>
181	194	$usernameTextbox
182	195	$myResponseFilter
	196	+ $showUnansweredFilter
183	197	<button type="submit" id="fbd-filters-set">$setFiltersMsg</button>
184	198	</form>
185	199	<a href="$whatIsURL" id="fbd-about">$whatIsMsg</a>
—	—	@@ -585,12 +599,25 @@
586	600	$option = array( 'LIMIT' => $limit + 2, 'ORDER BY' => "mbf_timestamp$desc, mbf_id$desc" );
587	601	$tableJoin = array( 'user' => array( 'LEFT JOIN', 'user_id=mbf_user_id' ) );
588	602
589		~~- //View my response~~
590		~~- if ( isset( $filters['myresponse'] ) && $filters['myresponse'] == '1' && !$wgUser->isAnon() ) {~~
591		~~- $table[] = 'moodbar_feedback_response';~~
592		~~- $option['GROUP BY'] = 'mbf_id';~~
593		~~- $tableJoin['moodbar_feedback_response'] = array( 'INNER JOIN', 'mbf_id=mbfr_mbf_id' );~~
594		~~- $conds['mbfr_user_id'] = $wgUser->getId();~~
	603	+
	604	+ // View my response or unanswered feedback
	605	+ if ( isset( $filters['responsefilter'] ) ) {
	606	+ switch ( $filters['responsefilter'] ) {
	607	+ case 'myresponse':
	608	+ if ( !$wgUser->isAnon() ) {
	609	+ $table[] = 'moodbar_feedback_response';
	610	+ $option['GROUP BY'] = 'mbf_id';
	611	+ $tableJoin['moodbar_feedback_response'] = array( 'INNER JOIN', 'mbf_id=mbfr_mbf_id' );
	612	+ $conds['mbfr_user_id'] = $wgUser->getId();
	613	+ }
	614	+ break;
	615	+
	616	+ case 'showunanswered':
	617	+ $table[] = 'moodbar_feedback_response';
	618	+ $tableJoin['moodbar_feedback_response'] = array( 'LEFT JOIN', 'mbf_id=mbfr_mbf_id' );
	619	+ $conds['mbfr_id'] = null;
	620	+ break;
	621	+ }
595	622	}
596	623
597	624	$res = $dbr->select( $table, array(
Index: trunk/extensions/MoodBar/MoodBar.i18n.php
—	—	@@ -110,6 +110,7 @@
111	111	'moodbar-feedback-filters-type-sad' => 'Sad',
112	112	'moodbar-feedback-filters-username' => 'Username:',
113	113	'moodbar-feedback-filters-my-response' => 'My responses only',
	114	+ 'moodbar-feedback-filters-show-unanswered' => 'Show unanswered',
114	115	'moodbar-feedback-filters-button' => 'Set filters',
115	116	'moodbar-feedback-whatis' => 'What is this feature?',
116	117	'moodbar-feedback-permalink' => 'link to here',
—	—	@@ -290,6 +291,7 @@
291	292	'moodbar-feedback-filters-type-sad' => 'Used on Special:FeedbackDashboard to filter by "sad" feedback entries',
292	293	'moodbar-feedback-filters-username' => '{{Identical\|Username}}',
293	294	'moodbar-feedback-filters-my-response' => 'Used on SpecialFeedbackDashboard to filter by "My responses only" feedback entries. A checkbox comes right before the text.',
	295	+ 'moodbar-feedback-filters-show-unanswered' => 'Used on SpecialFeedbackDashboard to filter by "Show unanswered" feedback entries',
294	296	'moodbar-feedback-more' => 'Text of the link that the user can click to see more results. Only visible if JavaScript is enabled.
295	297	{{Identical\|More}}',
296	298	'moodbar-feedback-newer' => 'Text of the link that the user can click to go back to more recent results. Only visible if JavaScript is not enabled.',
Index: trunk/extensions/MoodBar/ApiQueryMoodBarComments.php
—	—	@@ -38,11 +38,18 @@
39	39	$this->addWhereRange( 'mbf_id', $params['dir'], null, null );
40	40	$this->addOption( 'LIMIT', $params['limit'] + 1 );
41	41
42		~~- if ( $params['myresponse'] == '1' && !$wgUser->isAnon() ) {~~
	42	+
	43	+ if ( $params['myresponse'] == '1' ) {
	44	+ if ( !$wgUser->isAnon() ) {
	45	+ $this->addTables( array( 'moodbar_feedback_response' ) );
	46	+ $this->addJoinConds( array( 'moodbar_feedback_response' => array( 'INNER JOIN', 'mbf_id=mbfr_mbf_id' ) ) );
	47	+ $this->addWhereFld( 'mbfr_user_id', $wgUser->getId() );
	48	+ $this->addOption( 'GROUP BY', 'mbf_id' );
	49	+ }
	50	+ } elseif ( $params['showunanswered'] == '1' ) {
43	51	$this->addTables( array( 'moodbar_feedback_response' ) );
44		~~- $this->addJoinConds( array( 'moodbar_feedback_response' => array( 'INNER JOIN', 'mbf_id=mbfr_mbf_id' ) ) );~~
45		~~- $this->addWhereFld( 'mbfr_user_id', $wgUser->getId() );~~
46		~~- $this->addOption( 'GROUP BY', 'mbf_id' );~~
	52	+ $this->addJoinConds( array( 'moodbar_feedback_response' => array( 'LEFT JOIN', 'mbf_id=mbfr_mbf_id' ) ) );
	53	+ $this->addWhere( array( 'mbfr_id' => null ) );
47	54	}
48	55
49	56	if ( ! $wgUser->isAllowed( 'moodbar-admin' ) ) {
—	—	@@ -164,8 +171,11 @@
165	172	ApiBase::PARAM_TYPE => 'user',
166	173	),
167	174	'myresponse' => array(
168		~~- ApiBase::PARAM_TYPE => array( '1' , '0' ),~~
	175	+ ApiBase::PARAM_TYPE => array( '1', '0' ),
169	176	),
	177	+ 'showunanswered' => array(
	178	+ ApiBase::PARAM_TYPE => array( '1', '0' ),
	179	+ ),
170	180	'prop' => array(
171	181	ApiBase::PARAM_TYPE => array( 'metadata', 'formatted', 'hidden' ),
172	182	ApiBase::PARAM_DFLT => 'metadata',
—	—	@@ -185,6 +195,7 @@
186	196	'type' => 'Only return comments of the given type(s). If not set or empty, return all comments',
187	197	'user' => 'Only return comments submitted by the given user',
188	198	'myresponse' => 'Only return comments to which the current user has responded',
	199	+ 'showunanswered' => 'Only return comments to which no one has responded',
189	200	'prop' => array( 'Which properties to get:',
190	201	' metadata - Comment ID, type, timestamp, user',
191	202	' formatted - HTML that would be displayed for this comment on Special:MoodBarFeedback',
Index: trunk/extensions/MoodBar/modules/ext.moodBar.dashboard/ext.moodBar.dashboard.js
—	—	@@ -5,7 +5,7 @@
6	6	/**
7	7	* Saved form state
8	8	*/
9		~~- var formState = { types: [], username: '', myresponse: '0' };~~
	9	+ var formState = { types: [], username: '', myresponse: '0', showunanswered: '0' };
10	10
11	11	/**
12	12	* Save the current form state to formState
—	—	@@ -14,6 +14,7 @@
15	15	formState.types = getSelectedTypes();
16	16	formState.username = $( '#fbd-filters-username' ).val();
17	17	formState.myresponse = $( '#fbd-filters-my-response' ).prop( 'checked' ) ? $( '#fbd-filters-my-response' ).val() : '0';
	18	+ formState.showunanswered = $( '#fbd-filters-show-unanswered' ).prop( 'checked' ) ? $( '#fbd-filters-show-unanswered' ).val() : '0';
18	19	}
19	20
20	21	/**
—	—	@@ -135,6 +136,9 @@
136	137	if ( formState.myresponse != '0' ) {
137	138	reqData.mbcmyresponse = formState.myresponse;
138	139	}
	140	+ if ( formState.showunanswered != '0' ) {
	141	+ reqData.mbcshowunanswered = formState.showunanswered;
	142	+ }
139	143
140	144	$.ajax( {
141	145	'url': mw.util.wikiScript( 'api' ),

Sign-offs

User	Flag	Date
Johnduhart	inspected	19:41, 7 January 2012

Follow-up revisions

Revision	Commit summary	Author	Date
r108454	followup to -r108187 - Checking isset instead of 1/0 for checkbox, adding myr...	bsitu	18:45, 9 January 2012
r108535	followup to -r108187 - move checkbox outside of label, remove INNER JOIN and ...	bsitu	19:33, 10 January 2012
r108539	followup to -r108187 - Add mbfr_user_id as index for my response filter	bsitu	19:59, 10 January 2012
r108556	followup to -r108187 - Introducing new field mbf_latest_response for latest f...	bsitu	22:42, 10 January 2012

Comments

#Comment by Johnduhart (talk | contribs) 19:43, 7 January 2012

API code looks fine however,

+				ApiBase::PARAM_TYPE => array( '1', '0' ),

Instead of having a 0/1 parameter, make it so that it works based on if the parameter is set (This is how it's usually done)

#Comment by Catrope (talk | contribs) 16:02, 10 January 2012

+		$showUnansweredFilter = '<label for="fbd-filters-show-unanswered" id="fbd-filters-type-show-unanswered-label" class="fbd-filters-label">' . 
+								$showUnansweredCheckbox . $showUnansweredMsg . '</label>';

Why is the checkbox inside the label tag like that, shouldn't it be before it?

+						$conds['mbfr_user_id'] = $wgUser->getId();

There is no index on this field at all, please make sure the query is properly indexed.

+					$tableJoin['moodbar_feedback_response'] = array( 'LEFT JOIN', 'mbf_id=mbfr_mbf_id' );
+					$conds['mbfr_id'] = null;

This isn't a very nice query, it does a full table scan on moodbar_feedback. It would be nicer to have a boolean mbf_answered field.

Pro tip: you don't need to specify join conds for INNER JOINs, you can specify those implicitly by just adding 'mbf_id=mbfr_mbf_id' as a WHERE condition.

John's right, the API offers boolean parameters (based on presence/absence). You can specify these with 'showunanswered' => false, .

#Comment by Bsitu (talk | contribs) 17:41, 10 January 2012

Forget to add the index, :(

Status & tagging log

01:42, 1 March 2012 Krinkle (talk | contribs) changed the tags for r108187 [removed: frontend]
08:58, 18 January 2012 Siebrand (talk | contribs) changed the tags for r108187 [removed: moodbar]
17:37, 11 January 2012 Catrope (talk | contribs) changed the status of r108187 [removed: new added: resolved]
22:43, 10 January 2012 Bsitu (talk | contribs) changed the status of r108187 [removed: resolved added: new]
21:52, 10 January 2012 Catrope (talk | contribs) changed the status of r108187 [removed: fixme added: resolved]
16:02, 10 January 2012 Catrope (talk | contribs) changed the status of r108187 [removed: new added: fixme]
19:43, 7 January 2012 Johnduhart (talk | contribs) changed the tags for r108187 [removed: api]
18:37, 7 January 2012 Johnduhart (talk | contribs) changed the tags for r108187 [added: api]
18:34, 7 January 2012 Johnduhart (talk | contribs) changed the tags for r108187 [added: frontend]
19:22, 6 January 2012 MarkAHershberger (talk | contribs) changed the tags for r108187 [added: moodbar]