r109949 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109948‎ | r109949 | r109950 >
Date:20:33, 24 January 2012
Author:gregchiasson
Status:ok
Tags:aft 
Comment:
AFT5 - rearrange some HTML for designers. add support for removing hidden/deleted flags (partial support for 'needs oversight', but it's not available on the frontend). Change the way filters work to a) make sense, and b) close a few permissions loopholes that never should have existed. Fix a typo in permission name. Rework special.js to avoid some code replication.
Modified paths:
  • /trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.hooks.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/SpecialArticleFeedbackv5.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.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,7 +39,10 @@
4040 af_hide_count integer unsigned NOT NULL DEFAULT 0,
4141 af_delete_count integer unsigned NOT NULL DEFAULT 0,
4242 af_helpful_count integer unsigned NOT NULL DEFAULT 0,
43 - af_unhelpful_count integer unsigned NOT NULL DEFAULT 0
 43+ af_unhelpful_count integer unsigned NOT NULL DEFAULT 0,
 44+ -- Flag a message as requiring oversight
 45+ af_needs_oversight boolean NOT NULL DEFAULT FALSE
 46+
4447 ) /*$wgDBTableOptions*/;
4548 CREATE INDEX /*i*/af_page_user_token_id ON /*_*/aft_article_feedback (af_page_id, af_user_id, af_user_anon_token, af_id);
4649 CREATE INDEX /*i*/af_revision_id ON /*_*/aft_article_feedback (af_revision_id);
Index: trunk/extensions/ArticleFeedbackv5/sql/alter.sql
——@@ -72,3 +72,8 @@
7373
7474 -- added 1/19 (greg)
7575 ALTER TABLE aft_article_feedback ADD COLUMN af_unhelpful_count integer unsigned NOT NULL DEFAULT 0;
 76+
 77+-- added 1/24 (greg)
 78+INSERT INTO aft_article_filter_count(afc_page_id, afc_filter_name, afc_filter_count) SELECT af_page_id, 'deleted', COUNT(*) FROM aft_article_feedback WHERE af_delete_count > 0 GROUP BY af_page_id;
 79+INSERT INTO aft_article_filter_count(afc_page_id, afc_filter_name, afc_filter_count) SELECT af_page_id, 'unhelpful', COUNT(*) FROM aft_article_feedback WHERE af_helpful_count <= 0 GROUP BY af_page_id;
 80+ALTER TABLE aft_article_feedback ADD COLUMN af_needs_oversight boolean NOT NULL DEFAULT FALSE;
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php
——@@ -81,9 +81,12 @@
8282
8383 'articlefeedbackv5-form-optionid' => 'Option $1',
8484 'articlefeedbackv5-form-hide' => 'Hide this post ($1)',
 85+ 'articlefeedbackv5-form-unhide' => 'Show this post',
8586 'articlefeedbackv5-form-helpful' => 'Mark as helpful ($1)',
8687 'articlefeedbackv5-form-abuse' => 'Flag as abuse ($1)',
8788 'articlefeedbackv5-form-delete' => 'Delete',
 89+ 'articlefeedbackv5-form-oversight' => 'Mark for oversight',
 90+ 'articlefeedbackv5-form-undelete' => 'Show this post',
8891 'articlefeedbackv5-form-header' => 'Feedback #$1, at $2',
8992 'articlefeedbackv5-form1-header-found' => '{{GENDER:$1|$1}} found what they were looking for:',
9093 'articlefeedbackv5-form1-header-not-found' => '{{GENDER:$1|$1}} did not find what they were looking for:',
——@@ -101,6 +104,9 @@
102105 'articlefeedbackv5-hide-saved' => 'Hide flag saved',
103106 'articlefeedbackv5-unhelpful-saved' => 'Marked as unhelpful',
104107 'articlefeedbackv5-helpful-saved' => 'Marked as helpful',
 108+ 'articlefeedbackv5-unhide-saved' => 'Feedback shown',
 109+ 'articlefeedbackv5-undelete-saved' => 'Feedback shown',
 110+ 'articlefeedbackv5-oversight-saved' => 'Marked for oversight',
105111 'articlefeedbackv5-error-loading-feedback' => 'Error loading feedback',
106112 'articlefeedbackv5-invalid-feedback-id' => 'Invalid feedback ID',
107113 'articlefeedbackv5-invalid-feedback-flag' => 'Invalid feedback flag',
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
——@@ -119,8 +119,8 @@
120120 oldId = $.articleFeedbackv5special.sort;
121121
122122 // set direction = desc...
123 - $.articleFeedbackv5special.sort = id;
124 - $.articleFeedbackv5special.continue = null;
 123+ $.articleFeedbackv5special.sort = id;
 124+ $.articleFeedbackv5special.continue = null;
125125
126126 // unless we're flipping the direction on the current sort.
127127 if( id == oldId && $.articleFeedbackv5special.sortDirection == 'desc' ) {
——@@ -140,29 +140,12 @@
141141 return false;
142142 } );
143143
144 - $( '.articleFeedbackv5-abuse-link' ).live( 'click', function( e ) {
145 - $.articleFeedbackv5special.flagFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-abuse-link-' ), 'abuse' );
146 - return false;
147 - } );
148 - $( '.articleFeedbackv5-hide-link' ).live( 'click', function( e ) {
149 - $.articleFeedbackv5special.flagFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-hide-link-' ), 'hide' );
150 - return false;
151 - } );
152 - $( '.articleFeedbackv5-delete-link' ).live( 'click', function( e ) {
153 - $.articleFeedbackv5special.flagFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-delete-link-' ), 'delete' );
154 - return false;
155 - } );
156 - $( '.articleFeedbackv5-helpful-link' ).live( 'click', function( e ) {
157 - $.articleFeedbackv5special.flagFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-helpful-link-' ), 'helpful' );
158 - $(this).addClass('active');
159 -
160 - return false;
161 - } );
162 - $( '.articleFeedbackv5-unhelpful-link' ).live( 'click', function( e ) {
163 - $.articleFeedbackv5special.flagFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-unhelpful-link-' ), 'unhelpful' );
164 - $(this).addClass('active');
165 - return false;
166 - } );
 144+ $.each( ['unhide', 'undelete', 'oversight', 'hide', 'abuse', 'delete', 'helpful', 'unhelpful'],
 145+ function ( index, value ) {
 146+ $( '.articleFeedbackv5-' + value + '-link' ).live( 'click', function( e ) {
 147+ $.articleFeedbackv5special.flagFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-' + value + '-link-' ), value );
 148+ })
 149+ });
167150 }
168151
169152 // }}}
Index: trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.php
——@@ -39,6 +39,19 @@
4040 if ( !$record->af_id ) {
4141 // no-op, because this is already broken
4242 $error = 'articlefeedbackv5-invalid-feedback-id';
 43+ } elseif( $params['flagtype'] == 'unhide' ) {
 44+ // remove the hidden status
 45+ $update[] = 'af_hide_count = 0';
 46+ } elseif( $params['flagtype'] == 'unoversight' ) {
 47+ // remove the oversight flag
 48+ $update[] = 'af_needs_oversight = FALSE';
 49+ } elseif( $params['flagtype'] == 'undelete' ) {
 50+ // remove the deleted status, and clear oversight flag
 51+ $update[] = 'af_delete_count = 0';
 52+ $update[] = 'af_needs_oversight = FALSE';
 53+ } elseif( $params['flagtype'] == 'oversight' ) {
 54+ // flag for oversight
 55+ $update[] = 'af_oversight = TRUE';
4356 } elseif( in_array( $params['flagtype'], $flags ) ) {
4457 // Probably this doesn't need validation, since the API
4558 // will handle it, but if it's getting interpolated into
——@@ -169,7 +182,7 @@
170183 ApiBase::PARAM_REQUIRED => true,
171184 ApiBase::PARAM_ISMULTI => false,
172185 ApiBase::PARAM_TYPE => array(
173 - 'abuse', 'hide', 'helpful', 'unhelpful', 'delete' )
 186+ 'abuse', 'hide', 'helpful', 'unhelpful', 'delete', 'undelete', 'unhide', 'oversight' )
174187 ),
175188 );
176189 }
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
——@@ -200,25 +200,24 @@
201201 global $wgUser;
202202 $where = array();
203203
204 - // Permissions check
 204+ // Permissions check: these filters are for admins only.
205205 if(
206206 ( $filter == 'invisible'
207207 && !$wgUser->isAllowed( 'aftv5-see-hidden-feedback' ) )
208208 ||
209209 ( $filter == 'deleted'
210 - && !$wgUser->isAllowed( 'aftv5-ssee-deleted-feedback' ) )
 210+ && !$wgUser->isAllowed( 'aftv5-see-deleted-feedback' ) )
211211 ) {
212212 $filter = null;
213213 }
214214
215 - // Always limit this to non-hidden records, unless they
216 - // specifically ask to see them.
217 - if( in_array( $filter, array( 'invisible', 'all', 'almostall' ) ) ) {
 215+ // Don't let non-allowed users see these.
 216+ if( !$wgUser->isAllowed( 'aftv5-see-hidden-feedback' ) ) {
218217 $where['af_hide_count'] = 0;
219218 }
220219
221 - // Same, but for deleted/supressed/oversight-only records.
222 - if( in_array( $filter, array( 'deleted', 'all' ) ) ) {
 220+ // Don't let non-allowed users see these.
 221+ if( !$wgUser->isAllowed( 'aftv5-see-deleted-feedback' ) ) {
223222 $where['af_delete_count'] = 0;
224223 }
225224
——@@ -228,18 +227,16 @@
229228 $where[ 'af_id' ] = $filterValue;
230229 break;
231230 case 'all':
 231+ # relies on the above to handler filtering,
 232+ # because 'all' doesn't mean thhe same thing
 233+ # at all access levels.
232234 # oversight, real 'all' - no filtering done
233 - break;
234 - case 'almostall':
235235 # non-oversight 'all' - no deleted feedback
236 - $where['af_delete_count'] = 0;
237 - break;
238 - case 'notall':
239236 # non-moderator 'all' - no hidden/deleted
240 - $where['af_delete_count'] = 0;
241 - $where['af_hide_count'] = 0;
242237 break;
243238 case 'visible':
 239+ # For oversights/sysops to see what normal
 240+ # people see.
244241 $where['af_delete_count'] = 0;
245242 $where['af_hide_count'] = 0;
246243 break;
——@@ -352,6 +349,7 @@
353350 . Html::closeElement( 'p' );
354351
355352 // Don't render the toolbox if they can't do anything with it.
 353+ $tools = null;
356354 if( $can_hide || $can_delete ) {
357355 $tools = Html::openElement( 'div', array(
358356 'class' => 'articleFeedbackv5-feedback-tools',
——@@ -362,18 +360,42 @@
363361 ), wfMessage( 'articlefeedbackv5-form-tools-label' )->text() )
364362 . Html::openElement( 'ul', array(
365363 'id' => 'articleFeedbackv5-feedback-tools-list-'.$id
366 - ) )
367 - # TODO: unhide hidden posts
368 - . ( $can_hide ? Html::rawElement( 'li', array(), Html::element( 'a', array(
369 - 'id' => "articleFeedbackv5-hide-link-$id",
370 - 'class' => 'articleFeedbackv5-hide-link'
371 - ), wfMessage( 'articlefeedbackv5-form-hide', $record[0]->af_hide_count )->text() ) ) : '' )
372 - # TODO: nonoversight can mark for oversight, oversight can
373 - # either delete or un-delete, based on deletion status
374 - . ( $can_delete ? Html::rawElement( 'li', array(), Html::element( 'a', array(
375 - 'id' => "articleFeedbackv5-delete-link-$id",
376 - 'class' => 'articleFeedbackv5-delete-link'
377 - ), wfMessage( 'articlefeedbackv5-form-delete' )->text() ) ) : '' )
 364+ ) );
 365+
 366+
 367+ if( $can_hide ) {
 368+ $link = 'hide';
 369+ if( $record[0]->af_hide_count > 0 ) {
 370+ # unhide
 371+ $link = 'unhide';
 372+ }
 373+ $tools .= Html::rawElement( 'li', array(), Html::element( 'a', array(
 374+ 'id' => "articleFeedbackv5-$link-link-$id",
 375+ 'class' => "articleFeedbackv5-$link-link"
 376+ ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_hide_count )->text() ) );
 377+ }
 378+
 379+ # TODO: a third link, to remove oversight flag
 380+ if( $can_delete ) {
 381+ # delete
 382+ $link = 'delete';
 383+ if( $record[0]->af_delete_count > 0 ) {
 384+ # undelete
 385+ $link = 'undelete';
 386+ }
 387+# } else {
 388+# if( $record[0]->af_needs_oversight ) {
 389+# # TODO: already flagged
 390+# $link = 'oversight';
 391+# }
 392+# # flag for oversight
 393+# $link = 'oversight';
 394+ }
 395+
 396+ $tools .= Html::rawElement( 'li', array(), Html::element( 'a', array(
 397+ 'id' => "articleFeedbackv5-$link-link-$id",
 398+ 'class' => "articleFeedbackv5-$link-link"
 399+ ), wfMessage( "articlefeedbackv5-form-$link", $record[0]->af_delete_count )->text() ) )
378400 . Html::closeElement( 'ul' )
379401 . Html::closeElement( 'div' );
380402 }
——@@ -496,8 +518,8 @@
497519 return Html::openElement( 'h3', array(
498520 'class' => $class
499521 ) )
 522+ . Linker::link( Title::newFromText( $link ), $name )
500523 . Html::element( 'span', array( 'class' => 'icon' ) )
501 - . Linker::link( Title::newFromText( $link ), $name )
502524 . Html::element( 'span',
503525 array( 'class' => 'result' ),
504526 wfMessage( $message, $gender, $extra )->escaped()
——@@ -533,7 +555,7 @@
534556 ApiBase::PARAM_REQUIRED => false,
535557 ApiBase::PARAM_ISMULTI => false,
536558 ApiBase::PARAM_TYPE => array(
537 - 'all', 'invisible', 'visible', 'comment', 'id', 'helpful', 'unhelpful', 'abusive', 'almostall', 'notall' )
 559+ 'all', 'invisible', 'visible', 'comment', 'id', 'helpful', 'unhelpful', 'abusive', 'deleted' )
538560 ),
539561 'filtervalue' => array(
540562 ApiBase::PARAM_REQUIRED => false,
Index: trunk/extensions/ArticleFeedbackv5/SpecialArticleFeedbackv5.php
——@@ -17,7 +17,8 @@
1818 class SpecialArticleFeedbackv5 extends SpecialPage {
1919 private $filters = array(
2020 'comment',
21 - 'helpful'
 21+ 'helpful',
 22+ 'all'
2223 );
2324 private $sorts = array(
2425 'age',
——@@ -32,6 +33,12 @@
3334 global $wgUser;
3435 parent::__construct( 'ArticleFeedbackv5' );
3536
 37+ // NOTE: The 'all' option actually displays different things
 38+ // based on the users role, which is handled in the filter:
 39+ // - deleter-all is actually everything
 40+ // - hidder-all is 'visible + hidden'
 41+ // - regular non-admin all is just 'all visible'
 42+
3643 if( $wgUser->isAllowed( 'aftv5-see-hidden-feedback' ) ) {
3744 array_push( $this->filters,
3845 'invisible', 'unhelpful', 'abusive'
——@@ -39,23 +46,8 @@
4047 }
4148
4249 if( $wgUser->isAllowed( 'aftv5-see-deleted-feedback' ) ) {
43 - array_push( $this->filters, 'deleted', 'all' );
 50+ $this->filters[] = 'deleted';
4451 }
45 -
46 - // The 'all' option actually displays different things based
47 - // on the users role, which is why we do this.
48 - // - deleted-all is actually everything
49 - // - hidden-all is 'visible + hidden'
50 - // - regular non-admin all is just 'all visible'
51 - if( !$wgUser->isAllowed( 'aftv5-see-deleted-feedback' )
52 - && $wgUser->isAllowed( 'aftv5-see-hidden-feedback' ) ) {
53 - $this->filters[] = 'almostall';
54 - }
55 -
56 - if( !$wgUser->isAllowed( 'aftv5-see-deleted-feedback' )
57 - && !$wgUser->isAllowed( 'aftv5-see-hidden-feedback' ) ) {
58 - $this->filters[] = 'notall';
59 - }
6052 }
6153
6254 /**
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.hooks.php
——@@ -170,6 +170,9 @@
171171 'articlefeedbackv5-delete-saved',
172172 'articlefeedbackv5-helpful-saved',
173173 'articlefeedbackv5-unhelpful-saved',
 174+ 'articlefeedbackv5-unhide-saved',
 175+ 'articlefeedbackv5-undelete-saved',
 176+ 'articlefeedbackv5-oversight-saved',
174177 'articlefeedbackv5-comment-link',
175178 'articlefeedbackv5-special-sort-asc',
176179 'articlefeedbackv5-special-sort-desc'

Status & tagging log