r112610 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r112609‎ | r112610 | r112611 >
Date:16:22, 28 February 2012
Author:emsmith
Status:ok (Comments)
Tags:
Comment:
bug 34090 - change all sql updates to use escaping except for explicitly commented items, add logging of id of doer and timestamp for last hide/delete(oversight) of a feedback item, note this is not yet hooked into the front end js
Modified paths:
  • /trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.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
@@ -56,11 +56,14 @@
5757 af_has_comment boolean NOT NULL DEFAULT FALSE,
5858 -- Keep track of number of activities (hide/show/flag/unflag)
5959 -- should be equivalent to counting rows in logging table
60 - af_activity_count integer unsigned NOT NULL DEFAULT 0
61 - -- for some of the filtering, we need to know "unhidden"
62 - -- to do this we have to keep track of "has ever been hidden
63 - -- same with "has ever been oversighted, has ever had oversight requested"
64 - -- these go on and never go back off, really
 60+ af_activity_count integer unsigned NOT NULL DEFAULT 0,
 61+ -- keep the user id of the last hider and/or oversighter of the feedback
 62+ -- only registered users can do this, which is why no ips
 63+ -- data used on the overlay of hidden/oversighted items
 64+ af_hide_user_id integer unsigned NOT NULL DEFAULT 0,
 65+ af_hide_timestamp binary(14) NOT NULL DEFAULT '',
 66+ af_oversight_user_id integer unsigned NOT NULL DEFAULT 0,
 67+ af_oversight_timestamp binary(14) NOT NULL DEFAULT '',
6568 ) /*$wgDBTableOptions*/;
6669 CREATE INDEX /*i*/af_page_user_token_id ON /*_*/aft_article_feedback (af_page_id, af_user_id, af_user_anon_token, af_id);
6770 CREATE INDEX /*i*/af_revision_id ON /*_*/aft_article_feedback (af_revision_id);
Index: trunk/extensions/ArticleFeedbackv5/sql/alter.sql
@@ -133,6 +133,10 @@
134134 ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_is_undeleted BOOLEAN NOT NULL DEFAULT FALSE;
135135 ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_is_declined BOOLEAN NOT NULL DEFAULT FALSE;
136136 ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_activity_count integer unsigned NOT NULL DEFAULT 0;
 137+ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_hide_user_id integer unsigned NOT NULL DEFAULT 0;
 138+ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_oversight_user_id integer unsigned NOT NULL DEFAULT 0;
 139+ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_hide_timestamp binary(14) NOT NULL DEFAULT '';
 140+ALTER TABLE /*_*/aft_article_feedback ADD COLUMN af_oversight_timestamp binary(14) NOT NULL DEFAULT '';
137141
 142+-- set has_comment appropriately from current values
138143 UPDATE aft_article_feedback, aft_article_answer SET af_has_comment = TRUE WHERE af_bucket_id = 1 AND af_id = aa_feedback_id AND aa_response_text IS NOT NULL;
Index: trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.php
@@ -51,6 +51,7 @@
5252 // we use ONE db connection that talks to master
5353 $dbw = wfGetDB( DB_MASTER );
5454 $dbw->begin();
 55+ $timestamp = $dbw->timestamp();
5556
5657 // load feedback record, bail if we don't have one
5758 $record = $this->fetchRecord( $dbw, $feedbackId );
@@ -69,8 +70,11 @@
7071 $activity = 'oversight';
7172
7273 // delete
73 - $update[] = "af_is_deleted = TRUE";
74 - $update[] = "af_is_undeleted = FALSE";
 74+ $update['af_is_deleted'] = true;
 75+ $update['af_is_undeleted'] = false;
 76+ // only store the oversighter on delete/oversight
 77+ $update['af_oversight_user_id'] = $wgUser->getId();
 78+ $update['af_oversight_timestamp'] = $timestamp;
7579 // delete specific filters
7680 $filters['deleted'] = 1;
7781 $filters['notdeleted'] = -1;
@@ -80,17 +84,20 @@
8185
8286 // autohide if not hidden
8387 if (false == $record->af_is_hidden ) {
84 - $update[] = "af_is_hidden = TRUE";
85 - $update[] = "af_is_unhidden = FALSE";
 88+ $update['af_is_hidden'] = true;
 89+ $update['af_is_unhidden'] = false;
8690 $filters = $this->changeFilterCounts( $record, $filters, 'hide' );
 91+ // 0 is used for "autohidden" purposes, we'll explicitly set it to overwrite last hider
 92+ $update['af_hide_user_id'] = 0;
 93+ $update['af_hide_timestamp'] = $timestamp;
8794 $implicit_hide = true; // for logging
8895 }
8996
9097 } else {
9198 // decrease means "unoversight this" but does NOT auto-unhide
9299 $activity = 'unoversight';
93 - $update[] = "af_is_deleted = FALSE";
94 - $update[] = "af_is_undeleted = TRUE";
 100+ $update['af_is_deleted'] = false;
 101+ $update['af_is_undeleted'] = true;
95102 // increment "undeleted", decrement "deleted"
96103 // NOTE: we do not touch visible, since hidden controls visiblity
97104 $filters['deleted'] = -1;
@@ -106,17 +113,19 @@
107114 $activity = 'hidden';
108115
109116 // hide
110 - $update[] = "af_is_hidden = TRUE";
111 - $update[] = "af_is_unhidden = FALSE";
112 -
 117+ $update['af_is_hidden'] = true;
 118+ $update['af_is_unhidden'] = false;
 119+ // only store the hider on hide not show
 120+ $update['af_hide_user_id'] = $wgUser->getId();
 121+ $update['af_hide_timestamp'] = $timestamp;
113122 $filters = $this->changeFilterCounts( $record, $filters, 'hide' );
114123
115124 } else {
116125 // decrease means "unhide this"
117126 $activity = 'unhidden';
118127
119 - $update[] = "af_is_hidden = FALSE";
120 - $update[] = "af_is_unhidden = TRUE";
 128+ $update['af_is_hidden'] = false;
 129+ $update['af_is_unhidden'] = true;
121130
122131 $filters = $this->changeFilterCounts( $record, $filters, 'show' );
123132 }
@@ -125,9 +134,9 @@
126135
127136 $activity = 'decline';
128137 // oversight request count becomes 0
129 - $update[] = "af_oversight_count = 0";
 138+ $update['af_oversight_count'] = 0;
130139 // declined oversight is flagged
131 - $update[] = "af_is_declined = TRUE";
 140+ $update['af_is_declined'] = true;
132141 $filters['declined'] = 1;
133142 // if the oversight count was greater then 1
134143 if(0 < $record->af_oversight_count) {
@@ -160,14 +169,18 @@
161170 if($direction == 'increase') {
162171 $activity = 'flag';
163172 $filters['abusive'] = 1;
 173+ // NOTE: we are bypassing traditional sql escaping here
164174 $update[] = "af_abuse_count = af_abuse_count + 1";
165175
166176 // Auto-hide after threshold flags
167177 if( $record->af_abuse_count > $wgArticleFeedbackv5HideAbuseThreshold
168178 && false == $record->af_is_hidden ) {
169179 // hide
170 - $update[] = "af_is_hidden = TRUE";
171 - $update[] = "af_is_unhidden = FALSE";
 180+ $update['af_is_hidden'] = true;
 181+ $update['af_is_unhidden'] = false;
 182+ // 0 is used for "autohidden" purposes, we'll explicitly set it to overwrite last hider
 183+ $update['af_hide_user_id'] = 0;
 184+ $update['af_hide_timestamp'] = $timestamp;
172185
173186 $filters = $this->changeFilterCounts( $record, $filters, 'hide' );
174187 $results['abuse-hidden'] = 1;
@@ -179,12 +192,13 @@
180193 elseif($direction == 'decrease') {
181194 $activity = 'unflag';
182195 $filters['abusive'] = -1;
 196+ // NOTE: we are bypassing traditional sql escaping here
183197 $update[] = "af_abuse_count = GREATEST(CONVERT(af_abuse_count, SIGNED) -1, 0)";
184198
185199 // Un-hide if we don't have 5 flags anymore
186200 if( $record->af_abuse_count == 5 && true == $record->af_is_hidden ) {
187 - $update[] = "af_is_hidden = FALSE";
188 - $update[] = "af_is_unhidden = TRUE";
 201+ $update['af_is_hidden'] = false;
 202+ $update['af_is_unhidden'] = true;
189203
190204 $filters = $this->changeFilterCounts( $record, $filters, 'show' );
191205
@@ -201,24 +215,28 @@
202216 if($direction == 'increase') {
203217 $activity = 'request';
204218 $filters['needsoversight'] = 1;
 219+ // NOTE: we are bypassing traditional sql escaping here
205220 $update[] = "af_oversight_count = af_oversight_count + 1";
206221
207222 // autohide if not hidden
208223 if (false == $record->af_is_hidden ) {
209 - $update[] = "af_is_hidden = TRUE";
210 - $update[] = "af_is_unhidden = FALSE";
 224+ $update['af_is_hidden'] = true;
 225+ $update['af_is_unhidden'] = false;
 226+ // 0 is used for "autohidden" purposes, we'll explicitly set it to overwrite last hider
 227+ $update['af_hide_user_id'] = 0;
211228 $filters = $this->changeFilterCounts( $record, $filters, 'hide' );
212229 $implicit_hide = true; // for logging
213230 }
214231 } elseif($direction == 'decrease') {
215232 $activity = 'unrequest';
216233 $filters['needsoversight'] = -1;
 234+ // NOTE: we are bypassing traditional sql escaping here
217235 $update[] = "af_oversight_count = GREATEST(CONVERT(af_oversight_count, SIGNED) - 1, 0)";
218236
219237 // Un-hide if we don't have oversight flags anymore
220238 if( $record->af_oversight_count == 1 && true == $record->af_is_hidden ) {
221 - $update[] = "af_is_hidden = FALSE";
222 - $update[] = "af_is_unhidden = TRUE";
 239+ $update['af_is_hidden'] = false;
 240+ $update['af_is_unhidden'] = true;
223241
224242 $filters = $this->changeFilterCounts( $record, $filters, 'show' );
225243
@@ -243,6 +261,7 @@
244262 if( ( ($flag == 'helpful' && $direction == 'increase' )
245263 || ($flag == 'unhelpful' && $direction == 'decrease' ) )
246264 ) {
 265+ // NOTE: we are bypassing traditional sql escaping here
247266 $update[] = "af_helpful_count = af_helpful_count + 1";
248267 $update[] = "af_unhelpful_count = GREATEST(0, CONVERT(af_unhelpful_count, SIGNED) - 1)";
249268 $helpful++;
@@ -251,6 +270,7 @@
252271 } elseif ( ( ($flag == 'unhelpful' && $direction == 'increase' )
253272 || ($flag == 'helpful' && $direction == 'decrease' ) )
254273 ) {
 274+ // NOTE: we are bypassing traditional sql escaping here
255275 $update[] = "af_unhelpful_count = af_unhelpful_count + 1";
256276 $update[] = "af_helpful_count = GREATEST(0, CONVERT(af_helpful_count, SIGNED) - 1)";
257277 $helpful--;
@@ -260,15 +280,19 @@
261281 } else {
262282
263283 if ( 'unhelpful' === $flag && $direction == 'increase') {
 284+ // NOTE: we are bypassing traditional sql escaping here
264285 $update[] = "af_unhelpful_count = af_unhelpful_count + 1";
265286 $unhelpful++;
266287 } elseif ( 'unhelpful' === $flag && $direction == 'decrease') {
 288+ // NOTE: we are bypassing traditional sql escaping here
267289 $update[] = "af_unhelpful_count = GREATEST(0, CONVERT(af_unhelpful_count, SIGNED) - 1)";
268290 $unhelpful--;
269291 } elseif ( $flag == 'helpful' && $direction == 'increase' ) {
 292+ // NOTE: we are bypassing traditional sql escaping here
270293 $update[] = "af_helpful_count = af_helpful_count + 1";
271294 $helpful++;
272295 } elseif ( $flag == 'helpful' && $direction == 'decrease' ) {
 296+ // NOTE: we are bypassing traditional sql escaping here
273297 $update[] = "af_helpful_count = GREATEST(0, CONVERT(af_helpful_count, SIGNED) - 1)";
274298 $helpful--;
275299 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r112611bug 34090 - take out implicit show and add returning user that hid/oversighte...emsmith17:02, 28 February 2012
r112627bug 34090 - follow up to r112599 - change to use getText since it's going int...emsmith18:53, 28 February 2012
r112825bug 34090 - insert custom attributes (sigh) for the user and formatted timest...emsmith18:14, 1 March 2012
r112830bug 34090 - follow up to r111474 - use truncate for choppingemsmith19:42, 1 March 2012
r113052bug 34090 - remaining backend feature in requirements, oversight email genera...emsmith18:04, 5 March 2012
r113062bug 34090 - follow up to r110520 1. change index 2. default of null for conti...emsmith18:54, 5 March 2012
r113073bug 34090 - follow up to r111472 part 1 - change to use getDbKey, check for b...emsmith19:48, 5 March 2012
r113082bug 34090 - follow up to r111471 - changed to use text()emsmith20:42, 5 March 2012
r113083bug 34090 - follow up to rr111472 part 2 - only use log_id for orderingemsmith20:48, 5 March 2012
r113104bug 34090 - follow up to rr111472 part 3 - totally redo the continue function...emsmith23:10, 5 March 2012
r113159bug 34090 - follow up to rr111472 part 4 and follow up to r111596 (same issue...emsmith17:37, 6 March 2012
r113160bug 34090 - follow up to rr111472 part 5 plural and number format action countemsmith17:48, 6 March 2012
r113161bug 34090 - follow up to rr111472 part 6 last of lego messagesemsmith17:53, 6 March 2012
r113163bug 34090 - two additional configuration settings (help url and admin user ur...emsmith18:02, 6 March 2012
r113193bug 34090 - followup to r113104 - only sort by timestamp (sorting by log id w...emsmith22:56, 6 March 2012
r113228bug 34090 - followup to r113160emsmith14:05, 7 March 2012
r113247bug 34090 - db issue, remove one of the sorts from the query, use the ids arr...emsmith16:52, 7 March 2012
r113269bug 34090 - followup to r113247emsmith19:01, 7 March 2012
r113273bug 34090 - add javascript level hiding on request oversight IF autohidden is...emsmith19:22, 7 March 2012
r113287bug 34090 - usernames and formatted timestamps into red lines for hidden/over...emsmith20:22, 7 March 2012
r113311bug 34090 - fixing the username bugs - apparently using the data- stuff with ...emsmith22:34, 7 March 2012
r113317bug 34090 - js and css voodoo to make the element with the red lines appear a...emsmith23:01, 7 March 2012
r113370bug 34090 - make different titles for masking appear if it's been hidden or o...emsmith16:43, 8 March 2012
r113371bug 34090 - fixes for oversighter view for hide/oversight panelsemsmith17:51, 8 March 2012
r113383bug 34090 - not entirely necessary, but keeps $2 from showing up in hiders pa...emsmith19:25, 8 March 2012
r113384bug 34090 - fix for double red line issues when hiding an oversighted post ha...emsmith19:28, 8 March 2012
r113390bug 34090 - adding translation for "automatic hider" useremsmith19:44, 8 March 2012
r113392bug 34090 - followup to r113287 - adjusted localization documentationemsmith20:03, 8 March 2012
r113393bug 34090 - followup to r113370 - adjusted localization documentationemsmith20:05, 8 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r111211bug 34090 - Added a log namespace and log types for the Article Feedback v5 e...emsmith22:53, 10 February 2012
r111472bug 34090 - Added an additional column in the main feedback table to keep a c...emsmith19:14, 14 February 2012
r111474bug 34090 - remove a todo - take the note from the flag submission and save i...emsmith19:57, 14 February 2012
r111552bug 34090 - split activity notes into their own config for maximum length (th...emsmith16:07, 15 February 2012
r111557bug 34090 - changed structure of the link classes and ids per the following f...emsmith16:47, 15 February 2012
r111570bug 34090 - request oversight is now a counter - so you can request/unrequest...emsmith19:53, 15 February 2012
r111573bug 34090 - removed the "header" portion of the html generation from the acti...emsmith20:18, 15 February 2012
r111645bug 34090 - updated the filter count update script to get requested oversight...emsmith15:45, 16 February 2012
r112038bug 34090 - added additional counts for filters including unhidden, undeleted...emsmith20:28, 21 February 2012
r112039bug 34090 - fixed issue noted : if $feedback is false return nothing - not th...emsmith20:50, 21 February 2012
r112041bug 34090 - kill the -1 issue by never letting it get below 0emsmith21:08, 21 February 2012
r112115bug 34090 - no code changes, just fixing/adding keyword svn propertiesemsmith16:31, 22 February 2012
r112119bug 34090 - cast the naughty column so it can be signed, the greatest still k...emsmith16:57, 22 February 2012
r112142bug 34090 - toggle for atomic un/helpful changing (and elimination of an extr...emsmith20:37, 22 February 2012
r112147bug 34090 - note to self, watch the copy and paste errors...emsmith21:01, 22 February 2012
r112149bug 34090 - no upper limit, only lower limit - we can't get any worse then 0emsmith21:21, 22 February 2012
r112154bug 34090 - quick and dirty helper script to get missing documentation keysemsmith21:57, 22 February 2012
r112156bug 34090 - only send the activity header if continue < 1, make the more div ...emsmith22:14, 22 February 2012
r112161bug 34090 - fix limit and use old continue to determine if we do the headeremsmith23:15, 22 February 2012
r112218bug 34090 - make sure the name are right for hidden/unhidden logging (argh)emsmith16:44, 23 February 2012
r112225bug 34090 - unhidden and unoversight logic adjustmentsemsmith18:06, 23 February 2012
r112228bug 34090 - fix filter - needsoversight are always autohiddenemsmith19:16, 23 February 2012
r112230bug 34090 - let's try this again - needsoversight and declined will have hidd...emsmith19:38, 23 February 2012
r112232bug 34090 - hide and show shouldn't fiddle with oversight counts or declined ...emsmith19:48, 23 February 2012
r112599bug 34090 - follow up to r111211 - rename things to make them "less confusin...emsmith14:43, 28 February 2012
r112603bug 34090 - Add the logging of automated hide/show errors by the "Article Fee...emsmith15:21, 28 February 2012
r112604bug 34090 - can't believe there were no permissions checks in this - only del...emsmith15:38, 28 February 2012

Comments

#Comment by Catrope (talk | contribs)   22:04, 6 March 2012

The fact that you systematically changed all WHERE clauses to the array format and called out the ones that bypass it makes me very happy :)

Status & tagging log