r110182 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110181‎ | r110182 | r110183 >
Date:22:58, 27 January 2012
Author:gregchiasson
Status:ok (Comments)
Tags:aft 
Comment:
AFT5:
- Change hidden and deleted counters to boolean flags. Not sure why I used counters in the first place, if I'm honest.
- Check for falsnesss of $record in FlagFeedback API (fixes r110098)
- Enable sort by rating, which didn't work right before. In doing so, rewrote the queries in ViewFeedback almost entirely.
- Check that flags are above 0 when saving - they're unsigned ints, so negative numbers are bad.
Modified paths:
  • /trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php (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
——@@ -35,15 +35,14 @@
3636 -- Creation timetamp
3737 af_created binary(14) NOT NULL DEFAULT '',
3838 -- Number of times the feedback was hidden or marked as abusive.
39 - -- or flagged as helpful or unhelpful, or "deleted"/marked for oversight.
 39+ -- or flagged as helpful or unhelpful.
4040 af_abuse_count integer unsigned NOT NULL DEFAULT 0,
41 - af_hide_count integer unsigned NOT NULL DEFAULT 0,
42 - af_delete_count integer unsigned NOT NULL DEFAULT 0,
4341 af_helpful_count integer unsigned NOT NULL DEFAULT 0,
4442 af_unhelpful_count integer unsigned NOT NULL DEFAULT 0,
45 - -- Flag a message as requiring oversight
46 - af_needs_oversight boolean NOT NULL DEFAULT FALSE
47 -
 43+ -- Flag a message as requiring oversight, being hidden ,or being deleted
 44+ af_needs_oversight boolean NOT NULL DEFAULT FALSE,
 45+ af_is_deleted boolean NOT NULL DEFAULT FALSE,
 46+ af_is_hidden boolean NOT NULL DEFAULT FALSE
4847 ) /*$wgDBTableOptions*/;
4948 CREATE INDEX /*i*/af_page_user_token_id ON /*_*/aft_article_feedback (af_page_id, af_user_id, af_user_anon_token, af_id);
5049 CREATE INDEX /*i*/af_revision_id ON /*_*/aft_article_feedback (af_revision_id);
Index: trunk/extensions/ArticleFeedbackv5/sql/alter.sql
——@@ -105,3 +105,7 @@
106106 SELECT afrr_page_id, afrr_field_id, SUM(afrr_total), SUM(afrr_count)
107107 FROM aft_article_revision_feedback_ratings_rollup
108108 GROUP BY afrr_page_id;
 109+
 110+-- Added 1/27 (greg)
 111+ALTER TABLE aft_article_feedback CHANGE COLUMN af_delete_count af_is_deleted BOOLEAN NOT NULL DEFAULT FALSE;
 112+ALTER TABLE aft_article_feedback CHANGE COLUMN af_hide_count af_is_hidden BOOLEAN NOT NULL DEFAULT FALSE;
Index: trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.php
——@@ -27,60 +27,56 @@
2828 $flag = $params['flagtype'];
2929 $direction = isset( $params['direction'] ) ? $params['direction'] : 'increase';
3030 $counts = array( 'increment' => array(), 'decrement' => array() );
31 - $flags = array( 'abuse', 'hide', 'helpful', 'unhelpful', 'delete' );
 31+ $counters = array( 'abuse', 'helpful', 'unhelpful' );
 32+ $flags = array( 'oversight', 'hide', 'delete' );
3233 $results = array();
3334 $helpful = null;
3435 $error = null;
 36+ $where = array( 'af_id' => $params['feedbackid'] );
3537
3638 # load feedback record, bail if we don't have one
3739 $record = $this->fetchRecord( $params['feedbackid'] );
3840
39 - if ( !$record->af_id ) {
 41+ if ( $record === false || !$record->af_id ) {
4042 // no-op, because this is already broken
4143 $error = 'articlefeedbackv5-invalid-feedback-id';
42 - } elseif ( $params['flagtype'] == 'unhide' ) {
43 - if( $direction == 'increase' ) {
44 - // remove the hidden status
45 - $update[] = 'af_hide_count = 0';
46 - } else {
47 - // or set one
48 - $update[] = 'af_hide_count = 1';
 44+ } elseif ( in_array( $params['flagtype'], $flags ) ) {
 45+ switch( $params['flagtype'] ) {
 46+ case 'hide': $field = 'af_is_hidden'; break;
 47+ case 'oversight': $field = 'af_needs_oversight'; break;
 48+ case 'delete': $field = 'af_is_deleted'; break;
 49+ default: return; # return error, ideally.
4950 }
50 - } elseif ( $params['flagtype'] == 'unoversight' ) {
 51+
5152 if( $direction == 'increase' ) {
52 - // remove the oversight flag
53 - $update[] = 'af_needs_oversight = FALSE';
 53+ $update[] = "$field = TRUE";
5454 } else {
55 - // or set one
56 - $update[] = 'af_needs_oversight = TRUE';
57 - }
58 - } elseif ( $params['flagtype'] == 'undelete' ) {
59 - if( $direction == 'increase' ) {
60 - // remove the deleted status, and clear oversight flag
61 - $update[] = 'af_delete_count = 0';
62 - $update[] = 'af_needs_oversight = FALSE';
63 - } else {
64 - // add deleted status and oversight flag
65 - $update[] = 'af_delete_count = 1';
66 - $update[] = 'af_needs_oversight = TRUE';
67 - }
68 - } elseif ( $params['flagtype'] == 'oversight' ) {
69 - if( $direction == 'increase' ) {
70 - // flag for oversight
71 - $update[] = 'af_needs_oversight = TRUE';
72 - } else {
73 - // remove flag for oversight
74 - $update[] = 'af_needs_oversight = FALSE';
75 - }
76 - } elseif ( in_array( $params['flagtype'], $flags ) ) {
 55+ $update[] = "$field = FALSE";
 56+ }
 57+ } elseif ( in_array( $params['flagtype'], $counters ) ) {
7758 // Probably this doesn't need validation, since the API
7859 // will handle it, but if it's getting interpolated into
7960 // the SQL, I'm really wary not re-validating it.
8061 $field = 'af_' . $params['flagtype'] . '_count';
 62+
 63+ // Add another where condition to confirm that
 64+ // the new flag value is at or above 0 (we use
 65+ // unsigned ints, so negatives cause errors.
 66+
8167 if( $direction == 'increase' ) {
8268 $update[] = "$field = $field + 1";
 69+ // If this is already less than 0,
 70+ // don't do anything - it'll just
 71+ // throw a SQL error, so don't bother.
 72+ // Incrementing from 0 is still valid.
 73+ $where[] = "$field >= 0";
8374 } else {
8475 $update[] = "$field = $field - 1";
 76+ // If this is already 0 or less,
 77+ // don't decrement it, that would
 78+ // throw an error.
 79+ // Decrementing from 0 is not allowed.
 80+ $where[] = "$field > 0";
8581 }
8682 } else {
8783 $error = 'articlefeedbackv5-invalid-feedback-flag';
——@@ -100,7 +96,7 @@
10197
10298
10399 // Newly hidden record
104 - if ( $flag == 'hide' && $record->af_hide_count == 0 ) {
 100+ if ( $flag == 'hide' && $record->af_is_hidden == 0 ) {
105101 $counts['increment'][] = 'invisible';
106102 $counts['decrement'][] = 'visible';
107103 }
——@@ -111,7 +107,7 @@
112108 }
113109
114110 // Newly deleted record
115 - if ( $flag == 'delete' && $record->af_delete_count == 0 ) {
 111+ if ( $flag == 'delete' && $record->af_is_deleted == 0 ) {
116112 $counts['increment'][] = 'deleted';
117113 $counts['decrement'][] = 'visible';
118114 }
——@@ -136,7 +132,7 @@
137133 $dbw->update(
138134 'aft_article_feedback',
139135 $update,
140 - array( 'af_id' => $params['feedbackid'] ),
 136+ $where,
141137 __METHOD__
142138 );
143139
——@@ -182,7 +178,7 @@
183179 if( $record->af_abuse_count >= $wgArticleFeedbackv5HideAbuseThreshold ) {
184180 $dbw->update(
185181 'aft_article_feedback',
186 - array( 'af_hide_count = af_hide_count + 1' ),
 182+ array( 'af_is_hidden = af_is_hidden + 1' ),
187183 array( 'af_id' => $params['feedbackid'] ),
188184 __METHOD__
189185 );
——@@ -210,7 +206,7 @@
211207 $dbr = wfGetDB( DB_SLAVE );
212208 $record = $dbr->selectRow(
213209 'aft_article_feedback',
214 - array( 'af_id', 'af_abuse_count', 'af_hide_count', 'af_helpful_count', 'af_unhelpful_count', 'af_delete_count' ),
 210+ array( 'af_id', 'af_abuse_count', 'af_is_hidden', 'af_helpful_count', 'af_unhelpful_count', 'af_is_deleted' ),
215211 array( 'af_id' => $id )
216212 );
217213 return $record;
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
——@@ -73,13 +73,13 @@
7474 return $count ? $count : 0;
7575 }
7676
77 - public function fetchFeedback( $pageId,
78 - $filter = 'visible', $filterValue = null, $sort = 'age', $sortOrder = 'desc', $limit = 25, $continue = null ) {
 77+ public function fetchFeedback( $pageId, $filter = 'visible',
 78+ $filterValue = null, $sort = 'age', $sortOrder = 'desc',
 79+ $limit = 25, $continue = null ) {
7980 $dbr = wfGetDB( DB_SLAVE );
8081 $ids = array();
8182 $rows = array();
8283 $rv = array();
83 - $where = $this->getFilterCriteria( $filter, $filterValue );
8484
8585 $direction = strtolower( $sortOrder ) == 'asc' ? 'ASC' : 'DESC';
8686 $continueDirection = ( $direction == 'ASC' ? '>' : '<' );
——@@ -87,6 +87,21 @@
8888 $continueSql;
8989 $sortField;
9090
 91+ $ratingField = 0;
 92+ $commentField = 0;
 93+ // This is in memcache so I don't feel that bad re-fetching it.
 94+ // Needed to join in the comment and rating tables, for filtering
 95+ // and sorting, respectively.
 96+ foreach( ApiArticleFeedbackv5Utils::getFields() as $field ) {
 97+ if( $field['afi_bucket_id'] == 1 && $field['afi_name'] == 'comment' ) {
 98+ $commentField = $field['afi_id'];
 99+ }
 100+ if( $field['afi_bucket_id'] == 1 && $field['afi_name'] == 'found' ) {
 101+ $ratingField = $field['afi_id'];
 102+ }
 103+ }
 104+
 105+ // Build ORDER BY clause.
91106 switch( $sort ) {
92107 case 'helpful':
93108 $sortField = 'net_helpfulness';
——@@ -94,10 +109,8 @@
95110 $continueSql = "CONVERT(af_helpful_count, SIGNED) - CONVERT(af_unhelpful_count, SIGNED) $continueDirection";
96111 break;
97112 case 'rating':
98 -# TODO
99 -# disable because it's broken
100 -# $sortField = 'rating';
101 -# break;
 113+ $sortField = 'rating';
 114+ break;
102115 case 'age':
103116 # Default field, fall through
104117 default:
——@@ -107,40 +120,52 @@
108121 }
109122 $order = "$sortField $direction";
110123
 124+ // Build WHERE clause.
 125+ // Filter applied , if any:
 126+ $where = $this->getFilterCriteria( $filter, $filterValue );
 127+ // PageID:
111128 $where['af_page_id'] = $pageId;
112 -
113 - # This join is needed for the comment filter.
114 - $where[] = 'af_id = aa_feedback_id';
115 -
 129+ // Continue value, if any:
116130 if ( $continue !== null ) {
117131 $where[] = $continueSql.' '.intVal( $continue );
118132 }
119 -
120 - # This filter is per Fabrice, 1/25 - only show feedback from option 1
 133+ // Only show bucket 1 (per Fabrice on 1/25)
121134 $where['af_bucket_id'] = 1;
122135
 136+ // Fetch the feedback IDs we need.
123137 /* I'd really love to do this in one big query, but MySQL
124138 doesn't support LIMIT inside IN() subselects, and since
125139 we don't know the number of answers for each feedback
126140 record until we fetch them, this is the only way to make
127141 sure we get all answers for the exact IDs we want. */
128142 $id_query = $dbr->select(
129 - array(
 143+ array(
130144 'aft_article_feedback',
131 - 'aft_article_answer'
 145+ 'rating' => 'aft_article_answer',
 146+ 'comment' => 'aft_article_answer',
132147 ),
133148 array(
134 - 'DISTINCT af_id',
135 - 'CONVERT(af_helpful_count, SIGNED) - CONVERT(af_unhelpful_count, SIGNED) AS net_helpfulness'
 149+ 'af_id',
 150+ 'CONVERT(af_helpful_count, SIGNED) - CONVERT(af_unhelpful_count, SIGNED) AS net_helpfulness',
 151+ 'rating.aa_response_boolean AS rating'
136152 ),
137153 $where,
138154 __METHOD__,
139155 array(
140156 'LIMIT' => $limit,
141157 'ORDER BY' => $order
 158+ ),
 159+ array(
 160+ 'rating' => array(
 161+ 'LEFT JOIN',
 162+ 'rating.aa_feedback_id = af_id AND rating.aa_field_id = '.intval( $ratingField )
 163+ ),
 164+ 'comment' => array(
 165+ 'LEFT JOIN',
 166+ 'comment.aa_feedback_id = af_id AND comment.aa_field_id = '.intval( $commentField )
 167+ )
142168 )
143169 );
144 -
145170 foreach ( $id_query as $id ) {
146171 $ids[] = $id->af_id;
147172 $this->continue = $id->$sortField;
——@@ -151,40 +176,47 @@
152177 }
153178
154179 $rows = $dbr->select(
155 - array( 'aft_article_feedback', 'aft_article_answer',
156 - 'aft_article_field', 'aft_article_field_option',
157 - 'user', 'page'
 180+ array( 'aft_article_feedback',
 181+ 'rating' => 'aft_article_answer',
 182+ 'answer' => 'aft_article_answer',
 183+ 'aft_article_field',
 184+ 'aft_article_field_option', 'user', 'page'
158185 ),
159186 array( 'af_id', 'af_bucket_id', 'afi_name', 'afo_name',
160 - 'aa_response_text', 'aa_response_boolean',
161 - 'aa_response_rating', 'aa_response_option_id',
 187+ 'answer.aa_response_text', 'answer.aa_response_boolean',
 188+ 'answer.aa_response_rating', 'answer.aa_response_option_id',
162189 'afi_data_type', 'af_created', 'user_name',
163 - 'af_user_ip', 'af_hide_count', 'af_abuse_count',
 190+ 'af_user_ip', 'af_is_hidden', 'af_abuse_count',
164191 'af_helpful_count', 'af_unhelpful_count',
165 - 'af_delete_count', 'af_needs_oversight',
 192+ 'af_is_deleted', 'af_needs_oversight',
166193 '(SELECT COUNT(*) FROM ' . $dbr->tableName( 'revision' ) . ' WHERE rev_id > af_revision_id AND rev_page = ' . ( integer ) $pageId . ') AS age',
167194 'CONVERT(af_helpful_count, SIGNED) - CONVERT(af_unhelpful_count, SIGNED) AS net_helpfulness',
168 - 'page_latest', 'af_revision_id', 'page_title'
 195+ 'page_latest', 'af_revision_id', 'page_title',
 196+ 'rating.aa_response_boolean AS rating'
169197 ),
170198 array( 'af_id' => $ids ),
171199 __METHOD__,
172200 array( 'ORDER BY' => $order ),
173201 array(
174 - 'page' => array(
175 - 'JOIN', 'page_id = af_page_id'
 202+ 'rating' => array(
 203+ 'LEFT JOIN',
 204+ 'rating.aa_feedback_id = af_id AND rating.aa_field_id = '.intval( $ratingField )
 205+ ),
 206+ 'answer' => array(
 207+ 'LEFT JOIN', 'af_id = answer.aa_feedback_id'
176208 ),
177 - 'user' => array(
178 - 'LEFT JOIN', 'user_id = af_user_id'
179 - ),
180209 'aft_article_field' => array(
181 - 'LEFT JOIN', 'afi_id = aa_field_id'
 210+ 'LEFT JOIN', 'afi_id = answer.aa_field_id'
182211 ),
183 - 'aft_article_answer' => array(
184 - 'LEFT JOIN', 'af_id = aa_feedback_id'
185 - ),
186212 'aft_article_field_option' => array(
187213 'LEFT JOIN',
188 - 'aa_response_option_id = afo_option_id'
 214+ 'answer.aa_response_option_id = afo_option_id'
 215+ ),
 216+ 'user' => array(
 217+ 'LEFT JOIN', 'user_id = af_user_id'
 218+ ),
 219+ 'page' => array(
 220+ 'JOIN', 'page_id = af_page_id'
189221 )
190222 )
191223 );
——@@ -218,12 +250,12 @@
219251
220252 // Don't let non-allowed users see these.
221253 if ( !$wgUser->isAllowed( 'aftv5-see-hidden-feedback' ) ) {
222 - $where['af_hide_count'] = 0;
 254+ $where['af_is_hidden'] = 0;
223255 }
224256
225257 // Don't let non-allowed users see these.
226258 if ( !$wgUser->isAllowed( 'aftv5-see-deleted-feedback' ) ) {
227 - $where['af_delete_count'] = 0;
 259+ $where['af_is_deleted'] = 0;
228260 }
229261
230262 switch( $filter ) {
——@@ -234,22 +266,12 @@
235267 # Used for permalinks.
236268 $where['af_id'] = $filterValue;
237269 break;
238 - case 'all':
239 - # relies on the above to handler filtering,
240 - # because 'all' doesn't mean thhe same thing
241 - # at all access levels.
242 - # oversight, real 'all' - no filtering done
243 - # non-oversight 'all' - no deleted feedback
244 - # non-moderator 'all' - no hidden/deleted
245 - break;
246270 case 'visible':
247 - # For oversights/sysops to see what normal
248 - # people see.
249 - $where['af_delete_count'] = 0;
250 - $where['af_hide_count'] = 0;
 271+ $where['af_is_deleted'] = 0;
 272+ $where['af_is_hidden'] = 0;
251273 break;
252274 case 'invisible':
253 - $where[] = 'af_hide_count > 0';
 275+ $where[] = 'af_is_hidden > 0';
254276 break;
255277 case 'abusive':
256278 $where[] = 'af_abuse_count > 0';
——@@ -261,10 +283,10 @@
262284 $where[] = 'CONVERT(af_helpful_count, SIGNED) - CONVERT(af_unhelpful_count, SIGNED) < 0';
263285 break;
264286 case 'comment':
265 - $where[] = 'aa_response_text IS NOT NULL';
 287+ $where[] = 'comment.aa_response_text IS NOT NULL';
266288 break;
267289 case 'deleted':
268 - $where[] = 'af_delete_count > 0';
 290+ $where[] = 'af_is_deleted > 0';
269291 break;
270292 default:
271293 break;
——@@ -404,7 +426,7 @@
405427
406428 if ( $can_hide ) {
407429 $link = 'hide';
408 - if ( $record[0]->af_hide_count > 0 ) {
 430+ if ( $record[0]->af_is_hidden > 0 ) {
409431 # unhide
410432 $link = 'unhide';
411433 $tools .= Html::element( 'li', array(), wfMessage( 'articlefeedbackv5-hidden' ) );
——@@ -412,19 +434,19 @@
413435 $tools .= Html::rawElement( 'li', array(), Html::element( 'a', array(
414436 'id' => "articleFeedbackv5-$link-link-$id",
415437 'class' => "articleFeedbackv5-$link-link"
416 - ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_hide_count )->text() ) );
 438+ ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_is_hidden )->text() ) );
417439 }
418440
419441 if ( $can_delete ) {
420442 # delete
421443 $link = 'delete';
422 - if ( $record[0]->af_delete_count > 0 ) {
 444+ if ( $record[0]->af_is_deleted > 0 ) {
423445 $link = 'undelete';
424446 }
425447 $tools .= Html::rawElement( 'li', array(), Html::element( 'a', array(
426448 'id' => "articleFeedbackv5-$link-link-$id",
427449 'class' => "articleFeedbackv5-$link-link"
428 - ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_delete_count )->text() ) );
 450+ ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_is_deleted )->text() ) );
429451 }
430452
431453 /*
——@@ -444,7 +466,7 @@
445467 $tools .= Html::rawElement( 'li', array(), Html::element( 'a', array(
446468 'id' => "articleFeedbackv5-$link-link-$id",
447469 'class' => "articleFeedbackv5-$link-link"
448 - ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_delete_count )->text() ) );
 470+ ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_is_deleted )->text() ) );
449471 }
450472 */
451473

Follow-up revisions

RevisionCommit summaryAuthorDate
r110397AFT5 - Fix the way auto-hiding abuse comments is done: Add conditional as a q...gregchiasson15:58, 31 January 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r110098AFT5 - Backend plumbing in FlagFeedback call to support toggle buttons and de...gregchiasson00:11, 27 January 2012

Comments

#Comment by Catrope (talk | contribs)   07:48, 31 January 2012 
-					array( 'af_hide_count = af_hide_count + 1' ),
+					array( 'af_is_hidden = af_is_hidden + 1' ),

If is_hidden is a boolean now, why are you still incrementing it? Don't you mean to just set af_is_hidden = TRUE here?

Status & tagging log