r104923 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r104922‎ \| r104923 \| r104924 >
Date:	23:54, 1 December 2011
Author:	gregchiasson
Status:	resolved (Comments)
Tags:
Comment:	AFTv5 feedback page shows correct numbers for showing and total counts, and the more button goes away when there's no more to show.
Modified paths:	/trunk/extensions/ArticleFeedbackv5/SpecialArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
—	—	@@ -3,11 +3,12 @@
4	4
5	5	// TODO: Pass this in from the PHP side. Add it to mwConfig or w/e?
6	6	//$.articleFeedbackv5special.page = mw.config.get( 'wgPageId' );
7		~~- $.articleFeedbackv5special.page = hackPageId;~~
8		~~- $.articleFeedbackv5special.filter = 'all';~~
9		~~- $.articleFeedbackv5special.sort = 'newest';~~
10		~~- $.articleFeedbackv5special.limit = 5;~~
11		~~- $.articleFeedbackv5special.offset = 0;~~
	7	+ $.articleFeedbackv5special.page = hackPageId;
	8	+ $.articleFeedbackv5special.filter = 'all';
	9	+ $.articleFeedbackv5special.sort = 'newest';
	10	+ $.articleFeedbackv5special.limit = 5;
	11	+ $.articleFeedbackv5special.offset = 0;
	12	+ $.articleFeedbackv5special.showing = 0;
12	13
13	14	$.articleFeedbackv5special.apiUrl = mw.config.get( 'wgScriptPath' )
14	15	+ '/api.php';
—	—	@@ -24,7 +25,7 @@
25	26	$.articleFeedbackv5special.loadFeedback();
26	27	return false;
27	28	} );
28		~~- $( '#aft5-more' ).bind( 'click', function(e) {~~
	29	+ $( '#aft5-show-more' ).bind( 'click', function(e) {
29	30	$.articleFeedbackv5special.offset +=
30	31	$.articleFeedbackv5special.limit;
31	32	$.articleFeedbackv5special.loadFeedback();
—	—	@@ -90,13 +91,25 @@
91	92	'maxage' : 0,
92	93	},
93	94	'success': function ( data ) {
94		~~- // TODO check output and error if needed~~
95		~~- $( '#aft5-show-feedback' ).html(~~
96		~~-$( '#aft5-show-feedback' ).html() + data.query['articlefeedbackv5-view-feedback'].feedback~~
97		~~- );~~
	95	+ if ( 'data' in data ) {
	96	+ $( '#aft5-show-feedback' ).html(
	97	+ $( '#aft5-show-feedback' ).html() + data.data.feedback
	98	+ );
	99	+
	100	+ $.articleFeedbackv5special.showing += data.data.length;
	101	+ $( '#aft5-feedback-count-shown' ).html( $.articleFeedbackv5special.showing );
	102	+ $( '#aft5-feedback-count-total' ).html( data.data.count );
	103	+ if ( $.articleFeedbackv5special.showing >= data.data.count ) {
	104	+ $( '#aft5-show-more' ).hide();
	105	+ }
	106	+
	107	+ } else {
	108	+ // TODO: have error message
	109	+ }
98	110	}
99		~~- // TODO have a callback for failures.~~
	111	+ // TODO: have a callback for failures.
100	112	} );
	113	+
101	114	return false;
102	115	}
103	116	} )( jQuery );
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
—	—	@@ -27,11 +27,12 @@
28	28	*/
29	29	public function execute() {
30	30	$params = $this->extractRequestParams();
31		~~- #error_log(print_r($params,1));~~
32	31	$html = '';
33	32	$result = $this->getResult();
34		~~- $path = array( 'query', $this->getModuleName() );~~
35	33	$pageId = $params['pageid'];
	34	+ $length = 0;
	35	+ $count = $this->fetchFeedbackCount(
	36	+ $params['pageid'], $params['filter'] );
36	37	$feedback = $this->fetchFeedback(
37	38	$params['pageid'],
38	39	$params['filter'],
—	—	@@ -42,14 +43,12 @@
43	44
44	45	foreach ( $feedback as $record ) {
45	46	$html .= $this->renderFeedback($record);
	47	+ $length++;
46	48	}
47	49
48		~~- $result->addValue( $path, 'feedback', $html );~~
49		-
50		~~- $result->setIndexedTagName_internal(~~
51		~~- array( 'query', $this->getModuleName() ), 'aa'~~
52		~~- );~~
53		-
	50	+ $result->addValue( 'data', 'length', $length );
	51	+ $result->addValue( 'data', 'count', $count );
	52	+ $result->addValue( 'data', 'feedback', $html );
54	53	}
55	54
56	55	public function fetchOverallRating( $pageId ) {
—	—	@@ -79,14 +78,26 @@
80	79	return $rv;
81	80	}
82	81
	82	+ public function fetchFeedbackCount( $pageId, $filter ) {
	83	+ $dbr = wfGetDB( DB_SLAVE );
	84	+ $where = $this->getFilterCriteria( $filter );
83	85
	86	+ $where['af_page_id'] = $pageId;
	87	+
	88	+ return $dbr->selectField(
	89	+ array( 'aft_article_feedback' ),
	90	+ array( 'COUNT(*) AS count' ),
	91	+ $where
	92	+ );
	93	+ }
	94	+
84	95	public function fetchFeedback( $pageId,
85	96	$filter = 'visible', $order = 'newest', $limit = 5, $offset = 0 ) {
86	97	$dbr = wfGetDB( DB_SLAVE );
87	98	$ids = array();
88	99	$rows = array();
89	100	$rv = array();
90		~~- $where = array();~~
	101	+ $where = $this->getFilterCriteria( $filter );
91	102	$order;
92	103
93	104	switch($order) {
—	—	@@ -98,18 +109,6 @@
99	110	break;
100	111	}
101	112
102		~~- switch($filter) {~~
103		~~- case 'all':~~
104		~~- $where = array();~~
105		~~- break;~~
106		~~- case 'visible':~~
107		~~- $where = array( 'af_hide_count' => 0 );~~
108		~~- break;~~
109		~~- default:~~
110		~~- $where = array();~~
111		~~- break;~~
112		~~- }~~
113		-
114	113	$where['af_page_id'] = $pageId;
115	114
116	115	/* I'd really love to do this in one big query, but MySQL
—	—	@@ -144,7 +143,7 @@
145	144	array( 'ORDER BY' => $order ),
146	145	array(
147	146	'aft_article_field' => array(
148		~~- 'JOIN', 'afi_id = aa_field_id'~~
	147	+ 'LEFT JOIN', 'afi_id = aa_field_id'
149	148	),
150	149	'aft_article_answer' => array(
151	150	'LEFT JOIN', 'af_id = aa_feedback_id'
—	—	@@ -167,6 +166,22 @@
168	167	return $rv;
169	168	}
170	169
	170	+ private function getFilterCriteria( $filter ) {
	171	+ $where = array();
	172	+ switch($filter) {
	173	+ case 'all':
	174	+ $where = array();
	175	+ break;
	176	+ case 'visible':
	177	+ $where = array( 'af_hide_count' => 0 );
	178	+ break;
	179	+ default:
	180	+ $where = array();
	181	+ break;
	182	+ }
	183	+ return $where;
	184	+ }
	185	+
171	186	protected function renderFeedback( $record ) {
172	187	$id = $record[0]->af_id;
173	188	$rv = "<div class='aft5-feedback'><p>Feedback #$id"
—	—	@@ -178,7 +193,7 @@
179	194	case 4: $rv .= $this->renderBucket4( $record ); break;
180	195	case 5: $rv .= $this->renderBucket5( $record ); break;
181	196	case 6: $rv .= $this->renderBucket6( $record ); break;
182		~~- default: return 'Invalid bucket id';~~
	197	+ default: $rv .= $this->renderNoBucket( $record ); break;
183	198	}
184	199	$rv .= "<p>
185	200	<a href='#' class='aft5-hide-link' id='aft5-hide-link-$id'>Hide this (".$record[0]->af_hide_count.")</a>
—	—	@@ -236,6 +251,10 @@
237	252	return $this->renderBucket6( $record );
238	253	}
239	254
	255	+ private function renderNoBucket( $record ) {
	256	+ return 'Invalid form ID';
	257	+ }
	258	+
240	259	private function renderBucket6( $record ) {
241	260	return 'User was not shown a feedback form.';
242	261	}
Index: trunk/extensions/ArticleFeedbackv5/SpecialArticleFeedbackv5.php
—	—	@@ -60,14 +60,13 @@
61	61	-->
62	62	<br>
63	63	<span id="aft5-showing">
64		~~-Showing 0 posts (of 0)~~
	64	+Showing <span id="aft5-feedback-count-shown">0</span> posts (of <span id="aft5-feedback-count-total">0</span>)
65	65	</span>
66	66	<br>
67		~~-<div style="border:1px solid red;" id="aft5-show-feedback">Loading...</div>~~
68		~~-<a href="#" id="aft5-more">More</a>~~
	67	+<div style="border:1px solid red;" id="aft5-show-feedback"></div>
	68	+<a href="#" id="aft5-show-more">More</a>
69	69	EOH
70	70	);
71		~~- # "more" link to load the next 50 and append it.~~
72	71	}
73	72
74	73	protected static function formatNumber( $number ) {

Follow-up revisions

Revision	Commit summary	Author	Date
r105029	More internationalization on AFTv5 Feedback page.	gregchiasson	00:09, 3 December 2011

Comments

#Comment by Johnduhart (talk | contribs) 14:13, 2 December 2011

Spaces used for indents.

+	private function renderNoBucket( $record ) { 
+		return 'Invalid form ID';
+	}

Shouldn't that be localized?

#Comment by Gregchiasson (talk | contribs) 01:07, 9 December 2011

Fixed in r105029

#Comment by P858snake (talk | contribs) 06:10, 10 December 2011

Can someone un-fixme and manually add the follow up?

Status & tagging log

22:50, 13 December 2011 Catrope (talk | contribs) changed the status of r104923 [removed: fixme added: resolved]
14:13, 2 December 2011 Johnduhart (talk | contribs) changed the status of r104923 [removed: new added: fixme]