r105632 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r105631‎ \| r105632 \| r105633 >
Date:	00:46, 9 December 2011
Author:	gregchiasson
Status:	ok
Tags:
Comment:	Enable sorting/filtering on AFTv5 feedback page, use a proper prefix on variable names, kill a few errors, better JS error reporting (still not great).
Modified paths:	/trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js (modified) (history) /trunk/extensions/ArticleFeedbackv5/sql/ArticleFeedbackv5.sql (modified) (history) /trunk/extensions/ArticleFeedbackv5/sql/alter.sql (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedbackv5/sql/ArticleFeedbackv5.sql
—	—	@@ -39,6 +39,7 @@
40	40	-- Create an index on the article_feedback.af_timestamp field
41	41	CREATE INDEX /i/article_feedback_timestamp ON /_/aft_article_feedback (af_created);
42	42	CREATE INDEX /i/af_page_id ON /_/aft_article_feedback (af_page_id, af_created);
	43	+CREATE INDEX /i/af_page_feedback_id ON /_/aft_article_feedback (af_page_id, af_id);
43	44
44	45	-- Allows for organizing fields into fieldsets, for reporting or rendering.
45	46	-- A group is just a name and an ID.
Index: trunk/extensions/ArticleFeedbackv5/sql/alter.sql
—	—	@@ -36,3 +36,6 @@
37	37	ALTER TABLE aft_article_revision_feedback_ratings_rollup CHANGE COLUMN afrr_rating_id afrr_field_id integer unsigned NOT NULL;
38	38	ALTER TABLE aft_article_feedback_ratings_rollup CHANGE COLUMN arr_rating_id arr_field_id integer unsigned NOT NULL;
39	39	ALTER TABLE aft_article_feedback_select_rollup ADD COLUMN afsr_field_id int NOT NULL;
	40	+
	41	+-- added 12/8 (later)
	42	+CREATE INDEX /i/af_page_feedback_id ON /_/aft_article_feedback (af_page_id, af_id);
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
—	—	@@ -3,7 +3,7 @@
4	4
5	5	// TODO: Pass this in better from the PHP side.
6	6	$.articleFeedbackv5special.page = hackPageId;
7		~~- $.articleFeedbackv5special.filter = 'all';~~
	7	+ $.articleFeedbackv5special.filter = 'visible';
8	8	$.articleFeedbackv5special.sort = 'newest';
9	9	$.articleFeedbackv5special.limit = 25;
10	10	$.articleFeedbackv5special.offset = 0;
—	—	@@ -13,18 +13,18 @@
14	14	$.articleFeedbackv5special.setBinds = function() {
15	15	$( '#aft5-filter' ).bind( 'change', function(e) {
16	16	$.articleFeedbackv5special.filter = $(this).val();
17		~~- $.articleFeedbackv5special.loadFeedback();~~
	17	+ $.articleFeedbackv5special.loadFeedback( true );
18	18	return false;
19	19	} );
20	20	$( '#aft5-sort' ).bind( 'change', function(e) {
21	21	$.articleFeedbackv5special.sort = $(this).val();
22		~~- $.articleFeedbackv5special.loadFeedback();~~
	22	+ $.articleFeedbackv5special.loadFeedback( true );
23	23	return false;
24	24	} );
25	25	$( '#aft5-show-more' ).bind( 'click', function(e) {
26	26	$.articleFeedbackv5special.offset +=
27	27	$.articleFeedbackv5special.limit;
28		~~- $.articleFeedbackv5special.loadFeedback();~~
	28	+ $.articleFeedbackv5special.loadFeedback( false);
29	29	return false;
30	30	} );
31	31	$( '.aft5-abuse-link' ).live( 'click', function(e) {
—	—	@@ -78,37 +78,46 @@
79	79	return false;
80	80	}
81	81
82		~~- $.articleFeedbackv5special.loadFeedback = function () {~~
	82	+ // ie, on loading next page, don't reset the view, but on sort/filter
	83	+ // remove what was there and start over.
	84	+ $.articleFeedbackv5special.loadFeedback = function ( resetContents ) {
83	85	$.ajax( {
84	86	'url' : $.articleFeedbackv5special.apiUrl,
85	87	'type' : 'GET',
86	88	'dataType': 'json',
87	89	'data' : {
88		~~- 'afpageid': $.articleFeedbackv5special.page,~~
89		~~- 'affilter': $.articleFeedbackv5special.filter,~~
90		~~- 'afsort' : $.articleFeedbackv5special.sort,~~
91		~~- 'aflimit' : $.articleFeedbackv5special.limit,~~
92		~~- 'afoffset': $.articleFeedbackv5special.offset,~~
	90	+ 'afvfpageid': $.articleFeedbackv5special.page,
	91	+ 'afvffilter': $.articleFeedbackv5special.filter,
	92	+ 'afvfsort' : $.articleFeedbackv5special.sort,
	93	+ 'afvflimit' : $.articleFeedbackv5special.limit,
	94	+ 'afvfoffset': $.articleFeedbackv5special.offset,
93	95	'action' : 'query',
94	96	'format' : 'json',
95	97	'list' : 'articlefeedbackv5-view-feedback',
96	98	'maxage' : 0
97	99	},
98	100	'success': function ( data ) {
99		~~- if ( 'data' in data ) {~~
100		~~- $( '#aft5-show-feedback' ).append( data.data.feedback);~~
101		~~- $.articleFeedbackv5special.showing += data.data.length;~~
	101	+ if ( 'articlefeedbackv5-view-feedback' in data ) {
	102	+ if ( resetContents ) {
	103	+ $( '#aft5-show-feedback' ).html( data['articlefeedbackv5-view-feedback'].feedback);
	104	+ $.articleFeedbackv5special.showing = data['articlefeedbackv5-view-feedback'].length;
	105	+ } else {
	106	+ $( '#aft5-show-feedback' ).append( data['articlefeedbackv5-view-feedback'].feedback);
	107	+ $.articleFeedbackv5special.showing += data['articlefeedbackv5-view-feedback'].length;
	108	+ }
102	109	$( '#aft5-feedback-count-shown' ).text( $.articleFeedbackv5special.showing );
103		~~- $( '#aft5-feedback-count-total' ).text( data.data.count );~~
104		~~- if ( $.articleFeedbackv5special.showing >= data.data.count ) {~~
	110	+ $( '#aft5-feedback-count-total' ).text( data['articlefeedbackv5-view-feedback'].count );
	111	+ if ( $.articleFeedbackv5special.showing >= data['articlefeedbackv5-view-feedback'].count ) {
105	112	$( '#aft5-show-more' ).hide();
106	113	}
107	114
108	115	} else {
109	116	$( '#aft5-show-feedback' ).text( mw.msg( 'articlefeedbackv5-error-loading-feedback' ) );
110	117	}
	118	+ },
	119	+ 'failure': function ( data ) {
	120	+ $( '#aft5-show-feedback' ).text( mw.msg( 'articlefeedbackv5-error-loading-feedback' ) );
111	121	}
112		~~- // TODO: have a callback for failures.~~
113	122	} );
114	123
115	124	return false;
—	—	@@ -120,10 +129,7 @@
121	130	// I think it maky have been a race condition.
122	131	$.articleFeedbackv5special.apiUrl = mw.util.wikiScript('api');
123	132
124		~~- // Blank out the 'loading' text~~
125		~~- $( '#aft5-show-feedback' ).text( ' ' );~~
126		-
127	133	// Set up event binds and do initial data fetch.
128	134	$.articleFeedbackv5special.setBinds();
129		~~- $.articleFeedbackv5special.loadFeedback();~~
	135	+ $.articleFeedbackv5special.loadFeedback( true );
130	136	} );
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
—	—	@@ -19,7 +19,7 @@
20	20	* Constructor
21	21	*/
22	22	public function __construct( $query, $moduleName ) {
23		~~- parent::__construct( $query, $moduleName, 'af' );~~
	23	+ parent::__construct( $query, $moduleName, 'afvf' );
24	24	}
25	25
26	26	/**
—	—	@@ -46,9 +46,9 @@
47	47	$length++;
48	48	}
49	49
50		~~- $result->addValue( 'data', 'length', $length );~~
51		~~- $result->addValue( 'data', 'count', $count );~~
52		~~- $result->addValue( 'data', 'feedback', $html );~~
	50	+ $result->addValue( $this->getModuleName(), 'length', $length );
	51	+ $result->addValue( $this->getModuleName(), 'count', $count );
	52	+ $result->addValue( $this->getModuleName(), 'feedback', $html );
53	53	}
54	54
55	55	public function fetchFeedbackCount( $pageId, $filter ) {
—	—	@@ -75,6 +75,9 @@
76	76
77	77	# Newest first is the only option right now.
78	78	switch($order) {
	79	+ case 'oldest':
	80	+ $order = 'af_id ASC';
	81	+ break;
79	82	case 'newest':
80	83	default:
81	84	$order = 'af_id DESC';
—	—	@@ -100,6 +103,10 @@
101	104	$ids[] = $id->af_id;
102	105	}
103	106
	107	+ if( !count( $ids ) ) {
	108	+ return array();
	109	+ }
	110	+
104	111	$rows = $dbr->select(
105	112	array( 'aft_article_feedback', 'aft_article_answer',
106	113	'aft_article_field', 'aft_article_field_option',
—	—	@@ -149,11 +156,12 @@
150	157	case 'all':
151	158	$where = array();
152	159	break;
	160	+ case 'invisible':
	161	+ $where = array( 'af_hide_count > 0' );
	162	+ break;
153	163	case 'visible':
	164	+ default:
154	165	$where = array( 'af_hide_count' => 0 );
155		~~- break;~~
156		~~- default:~~
157		~~- $where = array();~~
158	166	break;
159	167	}
160	168	return $where;
—	—	@@ -267,13 +275,13 @@
268	276	ApiBase::PARAM_REQUIRED => false,
269	277	ApiBase::PARAM_ISMULTI => false,
270	278	ApiBase::PARAM_TYPE => array(
271		~~- 'oldest', 'newest', 'etc' )~~
	279	+ 'oldest', 'newest' )
272	280	),
273	281	'filter' => array(
274	282	ApiBase::PARAM_REQUIRED => false,
275	283	ApiBase::PARAM_ISMULTI => false,
276	284	ApiBase::PARAM_TYPE => array(
277		~~- 'all', 'hidden', 'visible' )~~
	285	+ 'all', 'invisible', 'visible' )
278	286	),
279	287	'limit' => array(
280	288	ApiBase::PARAM_REQUIRED => false,

Status & tagging log

20:16, 12 December 2011 Catrope (talk | contribs) changed the status of r105632 [removed: deferred added: ok]
09:04, 9 December 2011 Siebrand (talk | contribs) changed the status of r105632 [removed: new added: deferred]