r109075 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r109074‎ \| r109075 \| r109076 >
Date:	23:12, 16 January 2012
Author:	gregchiasson
Status:	resolved (Comments)
Tags:	aft
Comment:	AFT5 - Add ability to 'delete' (super-hide, visible to oversight only) comments. Started plumbing the bits to allow updating the count queries when hiding feedback, but i need to noodle that one over a bit more.
Modified paths:	/trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5Utils.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (modified) (history) /trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.css (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
—	—	@@ -36,6 +36,7 @@
37	37	-- Number of times the feedback was hidden or marked as abusive.
38	38	af_abuse_count integer unsigned NOT NULL DEFAULT 0,
39	39	af_hide_count integer unsigned NOT NULL DEFAULT 0,
	40	+ af_delete_count integer unsigned NOT NULL DEFAULT 0,
40	41	af_helpful_count integer unsigned NOT NULL DEFAULT 0
41	42	) /$wgDBTableOptions/;
42	43	CREATE INDEX /i/af_page_user_token_id ON /_/aft_article_feedback (af_page_id, af_user_id, af_user_anon_token, af_id);
Index: trunk/extensions/ArticleFeedbackv5/sql/alter.sql
—	—	@@ -59,3 +59,4 @@
60	60
61	61	-- added 1/16 (greg)
62	62	ALTER TABLE aft_article_feedback ADD COLUMN af_helpful_count integer unsigned NOT NULL DEFAULT 0;
	63	+ALTER TABLE aft_article_feedback ADD COLUMN af_delete_count integer unsigned NOT NULL DEFAULT 0;
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php
—	—	@@ -46,10 +46,13 @@
47	47	'articlefeedbackv5-form-tools-label' => 'Tools',
48	48	'articlefeedbackv5-form-helpful-label' => 'Is this feedback helpful?',
49	49	'articlefeedbackv5-special-add-feedback' => 'Add your feedback',
	50	+ 'articlefeedbackv5-special-filter-all' => 'All ($1)',
50	51	'articlefeedbackv5-special-filter-comment' => 'Comments only ($1)',
	52	+ 'articlefeedbackv5-special-filter-deleted' => 'Deleted ($1)',
	53	+ 'articlefeedbackv5-special-filter-abusive' => 'Abusive ($1)',
	54	+ 'articlefeedbackv5-special-filter-helpful' => 'Helpful ($1)',
51	55	'articlefeedbackv5-special-filter-visible' => 'Visible ($1)',
52	56	'articlefeedbackv5-special-filter-invisible' => 'Invisible ($1)',
53		~~- 'articlefeedbackv5-special-filter-all' => 'All ($1)',~~
54	57	'articlefeedbackv5-special-sort-newest' => 'Newest first',
55	58	'articlefeedbackv5-special-sort-oldest' => 'Oldest first',
56	59	'articlefeedbackv5-special-sort-label-before' => 'Order by:',
—	—	@@ -77,6 +80,7 @@
78	81	'articlefeedbackv5-form5-header' => '$1 rated this page:',
79	82	'articlefeedbackv5-form-not-shown' => 'User was not shown a feedback form.',
80	83	'articlefeedbackv5-form-invalid' => 'Invalid feedback form ID.',
	84	+ 'articlefeedbackv5-delet-saved' => '',
81	85	'articlefeedbackv5-abuse-saved' => 'Flagged as abuse',
82	86	'articlefeedbackv5-hide-saved' => 'Hide flag saved',
83	87	'articlefeedbackv5-helpful-saved' => 'Marked as helpful',
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.css
—	—	@@ -1,4 +1,3 @@
2	2	/* css here */
3	3	div.articleFeedbackv5-feedback { border:1px solid #CCC; background:#EEE; margin-top:10px; clear:both;}
4	4	div.articleFeedbackv5-feedback-tools { border:1px solid #CCD; background:#EEF; margin:10px; width:219px; float:right;}
5		~~-p.articleFeedbackv5-comment-head { border:1px solid #CCC; }~~
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
—	—	@@ -93,6 +93,10 @@
94	94	$.articleFeedbackv5special.hideFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-hide-link-' ) );
95	95	return false;
96	96	} );
	97	+ $( '.articleFeedbackv5-delete-link' ).live( 'click', function( e ) {
	98	+ $.articleFeedbackv5special.deleteFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-delete-link-' ) );
	99	+ return false;
	100	+ } );
97	101	$( '.articleFeedbackv5-helpful-link' ).live( 'click', function( e ) {
98	102	$.articleFeedbackv5special.helpfulFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-helpful-link-' ) );
99	103	return false;
—	—	@@ -110,7 +114,7 @@
111	115	// {{{ helpfulFeedback
112	116
113	117	/**
114		~~- * Hides a response~~
	118	+ * Flags a response as helpful
115	119	*
116	120	* @param id int the feedback id
117	121	*/
—	—	@@ -118,7 +122,17 @@
119	123	$.articleFeedbackv5special.flagFeedback( id, 'helpful' );
120	124	}
121	125
	126	+ // {{{ hideFeedback
122	127
	128	+ /**
	129	+ * "Deletes" a response - actually just super-hides so only admins see.
	130	+ *
	131	+ * @param id int the feedback id
	132	+ */
	133	+ $.articleFeedbackv5special.deleteFeedback = function ( id ) {
	134	+ $.articleFeedbackv5special.flagFeedback( id, 'delete' );
	135	+ }
	136	+
123	137	// {{{ hideFeedback
124	138
125	139	/**
—	—	@@ -157,6 +171,7 @@
158	172	'type' : 'POST',
159	173	'dataType': 'json',
160	174	'data' : {
	175	+ 'pageid' : $.articleFeedbackv5special.page,
161	176	'feedbackid': id,
162	177	'flagtype' : type,
163	178	'format' : 'json',
Index: trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.php
—	—	@@ -38,6 +38,7 @@
39	39	array( 'af_id' => $params['feedbackid'] )
40	40	);
41	41
	42	+ # TODO:
42	43	if ( !$record->af_id ) {
43	44	// no-op, because this is already broken
44	45	$error = 'articlefeedbackv5-invalid-feedback-id';
—	—	@@ -47,6 +48,8 @@
48	49	$update['af_hide_count'] = $record->af_hide_count + 1;
49	50	} elseif ( $params['flagtype'] == 'helpful' ) {
50	51	$update['af_helpful_count'] = $record->af_helpful_count + 1;
	52	+ } elseif ( $params['flagtype'] == 'delete' ) {
	53	+ $update['af_delete_count'] = $record->af_delete_count + 1;
51	54	} else {
52	55	$error = 'articlefeedbackv5-invalid-feedback-flag';
53	56	}
—	—	@@ -85,6 +88,11 @@
86	89	*/
87	90	public function getAllowedParams() {
88	91	return array(
	92	+ 'pageid' => array(
	93	+ ApiBase::PARAM_REQUIRED => true,
	94	+ ApiBase::PARAM_ISMULTI => false,
	95	+ ApiBase::PARAM_TYPE => 'integer'
	96	+ ),
89	97	'feedbackid' => array(
90	98	ApiBase::PARAM_REQUIRED => true,
91	99	ApiBase::PARAM_ISMULTI => false,
—	—	@@ -94,7 +102,7 @@
95	103	ApiBase::PARAM_REQUIRED => true,
96	104	ApiBase::PARAM_ISMULTI => false,
97	105	ApiBase::PARAM_TYPE => array(
98		~~- 'abuse', 'hide', 'helpful' )~~
	106	+ 'abuse', 'hide', 'helpful', 'delete' )
99	107	),
100	108	);
101	109	}
Index: trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5.php
—	—	@@ -267,54 +267,14 @@
268	268	}
269	269
270	270	# Update the overall number of records for this page.
271		~~- $this->updateFilterCount( $pageId, 'all' );~~
272		~~- # If the feedbackrecord had a comment, update that fitler count.~~
	271	+ ApiArticleFeedbackv5Utils::incrementFilterCount( $pageId, 'all' );
	272	+ # If the feedbackrecord had a comment, update that filter count.
273	273	if( $has_comment ) {
274		~~- $this->updateFilterCount( $pageId, 'comment' );~~
	274	+ ApiArticleFeedbackv5Utils::incrementFilterCount( $pageId, 'comment' );
275	275	}
276	276	}
277	277
278	278	/**
279		~~- * Update the feedback count rollup table~~
280		- *
281		~~- * @param $pageId int the page id~~
282		~~- * @param $filterName string the name of the filter to update~~
283		~~- */~~
284		~~- private function updateFilterCount( $pageId, $filterName ) {~~
285		~~- $dbw = wfGetDB( DB_MASTER );~~
286		-
287		~~- $dbw->begin();~~
288		-
289		~~- # Try to insert the record, but ignore failures.~~
290		~~- # Ensures the count row exists.~~
291		~~- $dbw->insert(~~
292		~~- 'aft_article_filter_counts',~~
293		~~- array(~~
294		~~- 'afc_page_id' => $pageId,~~
295		~~- 'afc_filter_name' => $filterName,~~
296		~~- 'afc_filter_count' => 0~~
297		~~- ),~~
298		~~- __METHOD__,~~
299		~~- array( 'IGNORE' )~~
300		~~- );~~
301		-
302		~~- # Update the count row, incrementing the count.~~
303		~~- $dbw->update(~~
304		~~- 'aft_article_filter_counts',~~
305		~~- array(~~
306		~~- 'afc_filter_count = afc_filter_count + 1'~~
307		~~- ),~~
308		~~- array(~~
309		~~- 'afc_page_id' => $pageId,~~
310		~~- 'afc_filter_name' => $filterName~~
311		~~- ),~~
312		~~- __METHOD__~~
313		~~- );~~
314		-
315		~~- $dbw->commit();~~
316		~~- }~~
317		-
318		- /**
319	279	* Update the rollup tables
320	280	*
321	281	* @param $page int the page id
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
—	—	@@ -80,7 +80,6 @@
81	81	$where = $this->getFilterCriteria( $filter );
82	82	$order;
83	83
84		~~- # Newest first is the only option right now.~~
85	84	# TODO: The SQL needs to handle all sorts of weird cases.
86	85	switch( $order ) {
87	86	case 'oldest':
—	—	@@ -95,6 +94,8 @@
96	95	}
97	96
98	97	$where['af_page_id'] = $pageId;
	98	+ # the join is needed for the comment
	99	+ $where[] = 'af_id = aa_feedback_id';
99	100
100	101	if( $continue !== null ) {
101	102	$where[] = "$continueSql $continue";
—	—	@@ -106,8 +107,12 @@
107	108	record until we fetch them, this is the only way to make
108	109	sure we get all answers for the exact IDs we want. */
109	110	$id_query = $dbr->select(
110		~~- 'aft_article_feedback', 'af_id', $where, __METHOD__,~~
111	111	array(
	112	+ 'aft_article_feedback',
	113	+ 'aft_article_answer'
	114	+ ),
	115	+ 'DISTINCT af_id', $where, __METHOD__,
	116	+ array(
112	117	'LIMIT' => $limit,
113	118	'ORDER BY' => $order
114	119	)
—	—	@@ -130,7 +135,7 @@
131	136	'aa_response_rating', 'aa_response_option_id',
132	137	'afi_data_type', 'af_created', 'user_name',
133	138	'af_user_ip', 'af_hide_count', 'af_abuse_count',
134		~~- 'af_helpful_count'~~
	139	+ 'af_helpful_count', 'af_delete_count'
135	140	),
136	141	array( 'af_id' => $ids ),
137	142	__METHOD__,
—	—	@@ -173,6 +178,9 @@
174	179	case 'invisible':
175	180	$where = array( 'af_hide_count > 0' );
176	181	break;
	182	+ case 'comment':
	183	+ $where = array( 'aa_response_text IS NOT NULL');
	184	+ break;
177	185	case 'visible':
178	186	default:
179	187	$where = array( 'af_hide_count' => 0 );
—	—	@@ -206,20 +214,20 @@
207	215
208	216	$footer_links = Html::openElement( 'p', array( 'class' => 'articleFeedbackv5-comment-foot' ) )
209	217	. Html::openElement( 'ul' )
	218	+ . ( $can_upvote ? Html::rawElement( 'li', array(), Html::element( 'a', array(
	219	+ 'id' => "articleFeedbackv5-helpful-link-$id",
	220	+ 'class' => 'articleFeedbackv5-helpful-link'
	221	+ ), wfMessage( 'articlefeedbackv5-form-helpful', $record[0]->af_helpful_count )->text() ) ) : '' )
210	222	. ( $can_flag ? Html::rawElement( 'li', array(), Html::element( 'a', array(
211	223	'id' => "articleFeedbackv5-abuse-link-$id",
212	224	'class' => 'articleFeedbackv5-abuse-link'
213	225	), wfMessage( 'articlefeedbackv5-form-abuse', $record[0]->af_abuse_count )->text() ) ) : '' )
214		~~- . ( $can_upvote ? Html::rawElement( 'li', array(), Html::element( 'a', array(~~
215		~~- 'id' => "articleFeedbackv5-helpful-link-$id",~~
216		~~- 'class' => 'articleFeedbackv5-helpful-link'~~
217		~~- ), wfMessage( 'articlefeedbackv5-form-helpful', $record[0]->af_helpful_count )->text() ) ) : '' )~~
218	226	. Html::closeElement( 'ul' )
219	227	. Html::closeElement( 'p' );
220	228
221		~~- $rate = Html::openElement( 'div', array( 'class' => 'articleFeedbackv5-feedback-rate' ) )~~
222		~~- . wfMessage( 'articlefeedbackv5-form-helpful-label' )->escaped()~~
223		~~- . Html::closeElement( 'div' );~~
	229	+# $rate = Html::openElement( 'div', array( 'class' => 'articleFeedbackv5-feedback-rate' ) )
	230	+# . wfMessage( 'articlefeedbackv5-form-helpful-label' )->escaped()
	231	+# . Html::closeElement( 'div' );
224	232
225	233
226	234	$tools = Html::openElement( 'div', array( 'class' => 'articleFeedbackv5-feedback-tools' ) )
Index: trunk/extensions/ArticleFeedbackv5/api/ApiArticleFeedbackv5Utils.php
—	—	@@ -149,5 +149,46 @@
150	150	}
151	151	return $rv;
152	152	}
	153	+
	154	+ /**
	155	+ * Increments the per-page-per-filter count rollups used on the feedback
	156	+ * page.
	157	+ *
	158	+ * @param $pageId int the ID of the page (page.page_id)
	159	+ * @param $filterName string name of the filter to increment
	160	+ */
	161	+ public static function incrementFilterCount( $pageId, $filterName ) {
	162	+ $dbw = wfGetDB( DB_MASTER );
	163	+
	164	+ $dbw->begin();
	165	+
	166	+ # Try to insert the record, but ignore failures.
	167	+ # Ensures the count row exists.
	168	+ $dbw->insert(
	169	+ 'aft_article_filter_count',
	170	+ array(
	171	+ 'afc_page_id' => $pageId,
	172	+ 'afc_filter_name' => $filterName,
	173	+ 'afc_filter_count' => 0
	174	+ ),
	175	+ __METHOD__,
	176	+ array( 'IGNORE' )
	177	+ );
	178	+
	179	+ # Update the count row, incrementing the count.
	180	+ $dbw->update(
	181	+ 'aft_article_filter_count',
	182	+ array(
	183	+ 'afc_filter_count = afc_filter_count + 1'
	184	+ ),
	185	+ array(
	186	+ 'afc_page_id' => $pageId,
	187	+ 'afc_filter_name' => $filterName
	188	+ ),
	189	+ __METHOD__
	190	+ );
	191	+
	192	+ $dbw->commit();
	193	+ }
153	194	}
154	195

Follow-up revisions

Revision	Commit summary	Author	Date
r109116	r109075: Ignore empty message	raymond	07:58, 17 January 2012
r109156	AFT5 - spell translations properly, and add them to the list of messages load...	gregchiasson	16:07, 17 January 2012
r109964	AFT5 - Add some qqq translation explanation, as a followup to r109075	gregchiasson	22:37, 24 January 2012

Comments

#Comment by Nikerabbit (talk | contribs) 08:41, 17 January 2012

Where is articlefeedbackv5-delet-saved used?

#Comment by Gregchiasson (talk | contribs) 15:06, 17 January 2012

That's a typo. It should be "articlefeedbackv5-delete-saved", and it's used in modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js (or it will be, when I fix it).

#Comment by Raymond (talk | contribs) 17:07, 17 January 2012

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

#Comment by Catrope (talk | contribs) 01:23, 26 January 2012

OK other than the message docs thing.

Status & tagging log

01:28, 13 June 2012 Krinkle (talk | contribs) changed the tags for r109075 [removed: nodeploy]
06:48, 31 January 2012 Catrope (talk | contribs) changed the status of r109075 [removed: new added: resolved]
23:16, 27 January 2012 Gregchiasson (talk | contribs) changed the status of r109075 [removed: fixme added: new]
23:51, 26 January 2012 MarkAHershberger (talk | contribs) changed the tags for r109075 [added: nodeploy]
00:49, 18 January 2012 Siebrand (talk | contribs) changed the tags for r109075 [added: aft]
08:41, 17 January 2012 Nikerabbit (talk | contribs) changed the status of r109075 [removed: new added: fixme]