r110068 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110067‎ | r110068 | r110069 >
Date:19:23, 26 January 2012
Author:gregchiasson
Status:resolved (Comments)
Tags:aft 
Comment:
AFT5:
- Fix some translation issues: remove a few unused messages (take them out of hooks.php as well), add missing qqq doucmentation for others.
- Tweak text for up/down voting message, per requirements
- Add active class in CSS/JS for clicked 'is this helpful' yes/no buttons, per requirements.
- Don't display sort direction arrow on page load, per requirements.
- Make permalink filter take affect immediately, instead of just changing URL and requiring a reload.
- Comment out AFT5 special page display (was uncommented in dev, but needs to be commented out for committing - fixes a bug in r108881).
- Escape $continue value before using it in a query. Fixes the other bug in r108881.
- Use relative timestamps ('2 hours 19 minutes ago') for feedback posts less than 48 hours old (Taken largely from the way Moodbar does it).
- Remove the old commented-out permalinks, since the timestamp is the new link .
- Only show the 'X edits since' links (goes to diff page) if there have been edits since.
- Stop double-escaping feedbackHead (r109254), and add gender back into the me
ssage.
- Replace 'all' filter with 'visible' - there is no 'all' filter anymore. Remo
ve 'needs oversight' filter, as the 'request oversight' functionality is off t
he table for initial release. Will remove the links to it as well, but this co
mmit only takes out the filter.
Modified paths:
  • /trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.hooks.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.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.css (modified) (history)
  • /trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js (modified) (history)

Diff [purge]

Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.i18n.php
——@@ -52,19 +52,17 @@
5353 'articlefeedbackv5-form-helpful-label' => 'Is this feedback helpful?',
5454 'articlefeedbackv5-form-helpful-yes-label' => 'Yes',
5555 'articlefeedbackv5-form-helpful-no-label' => 'No',
56 - 'articlefeedbackv5-form-helpful-votes' => '({{PLURAL:$1|1 response|$1 responses}}: {{PLURAL:$2|1 yes|$2 yes}} / {{PLURAL:$3|1 no|$3 no}})',
 56+ 'articlefeedbackv5-form-helpful-votes' => '{{PLURAL:$1|1 yes|$1 yes}} / {{PLURAL:$2|1 no|$2 no}}',
5757 'articlefeedbackv5-special-add-feedback' => 'Add your feedback',
5858 'articlefeedbackv5-special-filter-all' => 'All ($1)',
5959 'articlefeedbackv5-special-filter-comment' => 'Comments only ($1)',
60 - 'articlefeedbackv5-special-filter-abusive' => 'Abusive ($1)',
 60+ 'articlefeedbackv5-special-filter-abusive' => 'Flagged as abuse ($1)',
6161 'articlefeedbackv5-special-filter-helpful' => 'Helpful ($1)',
6262 'articlefeedbackv5-special-filter-unhelpful' => 'Unhelpful ($1)',
6363 'articlefeedbackv5-special-filter-needsoversight' => 'Oversight requested ($1)',
64 - 'articlefeedbackv5-special-filter-visible' => 'Visible ($1)',
 64+ 'articlefeedbackv5-special-filter-visible' => 'All visible ($1)',
6565 'articlefeedbackv5-special-filter-invisible' => 'Hidden ($1)',
6666 'articlefeedbackv5-special-filter-deleted' => 'Deleted ($1)',
67 - 'articlefeedbackv5-special-sort-asc' => '^',
68 - 'articlefeedbackv5-special-sort-desc' => 'v',
6967 'articlefeedbackv5-special-sort-age' => 'Date',
7068 'articlefeedbackv5-special-sort-helpful' => 'Helpful',
7169 'articlefeedbackv5-special-sort-rating' => 'Rating',
——@@ -74,6 +72,7 @@
7573 'articlefeedbackv5-special-filter-label-after' => '',
7674 'articlefeedbackv5-special-showing' => '{{PLURAL:$1|1 feedback post|$1 feedback posts }} on this article',
7775 'articlefeedbackv5-comment-link' => 'Permalink',
 76+ 'articleFeedbackv5-comment-ago' => '$1 ago',
7877 'articlefeedbackv5-updates-since' => '{{PLURAL:$1|1 edit|$1 edits}} since post',
7978 'articlefeedbackv5-special-more' => 'Show more posts',
8079 'articlefeedbackv5-special-pagetitle' => 'Feedback: $1',
——@@ -102,8 +101,8 @@
103102 'articlefeedbackv5-delete-saved' => 'Feedback deleted',
104103 'articlefeedbackv5-abuse-saved' => 'Flagged as abuse',
105104 'articlefeedbackv5-hide-saved' => 'Hide flag saved',
106 - 'articlefeedbackv5-unhelpful-saved' => 'Marked as unhelpful',
107 - 'articlefeedbackv5-helpful-saved' => 'Marked as helpful',
 105+ 'articlefeedbackv5-unhelpful-saved' => 'No',
 106+ 'articlefeedbackv5-helpful-saved' => 'Yes',
108107 'articlefeedbackv5-unhide-saved' => 'Feedback shown',
109108 'articlefeedbackv5-undelete-saved' => 'Feedback shown',
110109 'articlefeedbackv5-oversight-saved' => 'Marked for oversight',
——@@ -331,6 +330,13 @@
332331 'articlefeedbackv5-special-filter-deleted' => '{{Identical|Deleted}}',
333332 'articlefeedbackv5-special-sort-age' => '{{Identical|Date}}',
334333 'articlefeedbackv5-special-sort-rating' => '{{Identical|Rating}}',
 334+ 'articlefeedbackv5-special-sort-label-before' => 'Place to put a label before the sort options',
 335+ 'articlefeedbackv5-special-sort-label-after' => 'Place to put a label after the sort options',
 336+ 'articlefeedbackv5-special-filter-label-before' => 'Place to put a label before the filter select box',
 337+ 'articlefeedbackv5-special-filter-label-after' => 'Place to put a label after the filter select box',
 338+ 'articlefeedbackv5-special-showing' => 'Text to show how many feedback psots have been posted to this article. $1 is the number of posts (needs plural support)',
 339+ 'articleFeedbackv5-comment-ago' => 'For posts less than 48 hours old, display a relative timestamp ("2 hours 19 minutes ago", eg). Formatting timestamp is in $1',
 340+ 'articlefeedbackv5-updates-since' => 'Number of edits made to this article since this feedback was posted. $1 is the number of edits. Requires plural support',
335341 'articlefeedbackv5-special-more' => '{{Identical|More}}',
336342 'articlefeedbackv5-special-pagetitle' => 'Page title for [[Special:ArticleFeedbackv5]]. Parameters:
337343 * $1 is the title of the article for which we show the feedback',
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.css
——@@ -321,3 +321,8 @@
322322 .articleFeedbackv5-comment-full {
323323 display: none;
324324 }
 325+
 326+a.helpful-active {
 327+ background: url(images/bg-button.png) !important;
 328+ color: white !important;
 329+}
Index: trunk/extensions/ArticleFeedbackv5/modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.special.js
——@@ -98,7 +98,6 @@
9999
100100 // Initial load
101101 $.articleFeedbackv5special.loadFeedback( true );
102 - $.articleFeedbackv5special.drawSortArrow();
103102 };
104103
105104 // }}}
——@@ -142,14 +141,31 @@
143142 return false;
144143 } );
145144
 145+ $( '.articleFeedbackv5-permalink' ).live( 'click', function( e ) {
 146+ id = $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-permalink-' );
 147+ $.articleFeedbackv5special.filter = 'id';
 148+ $.articleFeedbackv5special.filterValue = id;
 149+ $.articleFeedbackv5special.continue = null;
 150+ $.articleFeedbackv5special.loadFeedback( true );
 151+ } );
 152+
146153 $( '.articleFeedbackv5-comment-toggle' ).live( 'click', function( e ) {
147154 $.articleFeedbackv5special.toggleComment( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-comment-toggle-' ) );
148155 return false;
149156 } );
150157
151 - $.each( ['unhide', 'undelete', 'oversight', 'hide', 'abuse', 'delete', 'helpful', 'unhelpful', 'unoversight'],
152 - function ( index, value ) {
 158+ // Helpful and unhelpful have their own special logic, so break those out.
 159+ $.each( ['helpful', 'unhelpful' ], function ( index, value ) {
153160 $( '.articleFeedbackv5-' + value + '-link' ).live( 'click', function( e ) {
 161+ id = $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-' + value + '-link-' );
 162+ $.articleFeedbackv5special.flagFeedback( id, value );
 163+ // add highlighted class
 164+ $( this ).addClass( 'helpful-active' );
 165+ } )
 166+ } );
 167+
 168+ $.each( ['unhide', 'undelete', 'oversight', 'hide', 'abuse', 'delete', 'unoversight'], function ( index, value ) {
 169+ $( '.articleFeedbackv5-' + value + '-link' ).live( 'click', function( e ) {
154170 $.articleFeedbackv5special.flagFeedback( $.articleFeedbackv5special.stripID( this, 'articleFeedbackv5-' + value + '-link-' ), value );
155171 } )
156172 } );
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.php
——@@ -242,14 +242,14 @@
243243 $wgHooks['ArticleSaveComplete'][] = 'ArticleFeedbackv5Hooks::trackEditSuccess';
244244
245245 // API Registration
246 -$wgAPIListModules['articlefeedbackv5-view-ratings'] = 'ApiViewRatingsArticleFeedbackv5';
247 -$wgAPIListModules['articlefeedbackv5-view-feedback'] = 'ApiViewFeedbackArticleFeedbackv5';
248 -$wgAPIModules['articlefeedbackv5-flag-feedback'] = 'ApiFlagFeedbackArticleFeedbackv5';
 246+#$wgAPIListModules['articlefeedbackv5-view-ratings'] = 'ApiViewRatingsArticleFeedbackv5';
 247+#$wgAPIListModules['articlefeedbackv5-view-feedback'] = 'ApiViewFeedbackArticleFeedbackv5';
 248+#$wgAPIModules['articlefeedbackv5-flag-feedback'] = 'ApiFlagFeedbackArticleFeedbackv5';
249249 $wgAPIModules['articlefeedbackv5'] = 'ApiArticleFeedbackv5';
250250
251251 // Special Page
252 -$wgSpecialPages['ArticleFeedbackv5'] = 'SpecialArticleFeedbackv5';
253 -$wgSpecialPageGroups['ArticleFeedbackv5'] = 'other';
 252+#$wgSpecialPages['ArticleFeedbackv5'] = 'SpecialArticleFeedbackv5';
 253+#$wgSpecialPageGroups['ArticleFeedbackv5'] = 'other';
254254
255255 $wgAvailableRights[] = 'aftv5-hide-feedback';
256256 $wgAvailableRights[] = 'aftv5-delete-feedback';
Index: trunk/extensions/ArticleFeedbackv5/api/ApiFlagFeedbackArticleFeedbackv5.php
——@@ -141,8 +141,7 @@
142142 $unhelpful = $record->af_unhelpful_count;
143143
144144 $helpful = wfMessage( 'articlefeedbackv5-form-helpful-votes',
145 - ( $helpful + $unhelpful ),
146 - $helpful, $unhelpful
 145+ $helpful, $unhelpful
147146 )->escaped();
148147 }
149148 }
Index: trunk/extensions/ArticleFeedbackv5/api/ApiViewFeedbackArticleFeedbackv5.php
——@@ -112,7 +112,7 @@
113113 $where[] = 'af_id = aa_feedback_id';
114114
115115 if ( $continue !== null ) {
116 - $where[] = "$continueSql $continue";
 116+ $where[] = $continueSql.' '.intVal( $continue );
117117 }
118118
119119 /* I'd really love to do this in one big query, but MySQL
——@@ -269,7 +269,7 @@
270270 }
271271
272272 protected function renderFeedback( $record ) {
273 - global $wgArticlePath, $wgUser;
 273+ global $wgArticlePath, $wgUser, $wgLang;
274274 $id = $record[0]->af_id;
275275
276276 switch( $record[0]->af_bucket_id ) {
——@@ -282,13 +282,27 @@
283283 default: $content .= $this->renderNoBucket( $record ); break;
284284 }
285285
286 - // These two are the same for now, but may now always be,
 286+ // These two are the same for now, but may not always be,
287287 // so set them each separately.
288288 $can_flag = !$wgUser->isBlocked();
289289 $can_vote = !$wgUser->isBlocked();
290290 $can_hide = $wgUser->isAllowed( 'aftv5-hide-feedback' );
291291 $can_delete = $wgUser->isAllowed( 'aftv5-delete-feedback' );
292292
 293+ // Taken from the Moodbar extension.
 294+ $now = wfTimestamp( TS_UNIX );
 295+ $timestamp = wfTimestamp( TS_UNIX, $record[0]->af_created );
 296+ // Relative dates for 48 hours, normal timestamps later.
 297+ if( $timestamp > ( $now - ( 86400 * 2 ) ) ) {
 298+ // TODO: relative dates.
 299+ $time = $wgLang->formatTimePeriod(
 300+ ( $now - $timestamp ), 'avoidseconds'
 301+ );
 302+ $date = wfMessage( 'articleFeedbackv5-comment-ago', $time )->escaped();
 303+ } else {
 304+ $date = $wgLang->timeanddate($record[0]->af_created );
 305+ }
 306+
293307 $details = Html::openElement( 'div', array(
294308 'class' => 'articleFeedbackv5-comment-details'
295309 ) )
——@@ -296,32 +310,29 @@
297311 'class' => 'articleFeedbackv5-comment-details-date'
298312 ) )
299313 . Html::element( 'a', array(
300 - 'href' => "#id=$id"
301 - ), date( 'M j, Y H:i', strtotime( $record[0]->af_created ) ) )
 314+ 'class' => 'articleFeedbackv5-permalink',
 315+ 'id' => "articleFeedbackv5-permalink-$id",
 316+ 'href' => "#id=$id"
 317+ ), $date )
302318 . Html::closeElement( 'div' )
303 -# Remove for now, pending feedback.
304 -# . Html::openElement( 'div', array(
305 -# 'class' => 'articleFeedbackv5-comment-details-permalink'
306 -# ) )
307 -# .Html::element( 'a', array(
308 -# 'href' => "#id=$id"
309 -# ), wfMessage( 'articlefeedbackv5-comment-link' ) )
310 -# . Html::closeElement( 'div' )
311 -
312319 . Html::openElement( 'div', array(
313320 'class' => 'articleFeedbackv5-comment-details-updates'
314 - ) )
315 - . Linker::link(
316 - Title::newFromText( $record[0]->page_title ),
317 - wfMessage( 'articlefeedbackv5-updates-since', $record[0]->age ),
318 - array(),
319 - array(
320 - 'action' => 'historysubmit',
321 - 'diff' => $record[0]->page_latest,
322 - 'oldid' => $record[0]->af_revision_id
323 - )
324 - )
325 - . Html::closeElement( 'div' )
 321+ ) );
 322+
 323+ if( $record[0]->age > 0 ) {
 324+ $details .= Linker::link(
 325+ Title::newFromText( $record[0]->page_title ),
 326+ wfMessage( 'articlefeedbackv5-updates-since', $record[0]->age ),
 327+ array(),
 328+ array(
 329+ 'action' => 'historysubmit',
 330+ 'diff' => $record[0]->page_latest,
 331+ 'oldid' => $record[0]->af_revision_id
 332+ )
 333+ );
 334+ }
 335+
 336+ $details .= Html::closeElement( 'div' )
326337 . Html::closeElement( 'div' );
327338 ;
328339
——@@ -346,7 +357,7 @@
347358 $footer_links .= Html::element( 'span', array(
348359 'class' => 'articleFeedbackv5-helpful-votes',
349360 'id' => "articleFeedbackv5-helpful-votes-$id"
350 - ), wfMessage( 'articlefeedbackv5-form-helpful-votes', ( $record[0]->af_helpful_count + $record[0]->af_unhelpful_count ), $record[0]->af_helpful_count, $record[0]->af_unhelpful_count ) );
 361+ ), wfMessage( 'articlefeedbackv5-form-helpful-votes', $record[0]->af_helpful_count, $record[0]->af_unhelpful_count ) );
351362 if ( $can_flag ) {
352363 $footer_links .= Html::element( 'a', array(
353364 'id' => "articleFeedbackv5-abuse-link-$id",
——@@ -570,18 +581,18 @@
571582 }
572583
573584 private function feedbackHead( $message, $class, $record, $extra = '' ) {
574 - $gender = ''; # ?
575585 $name = htmlspecialchars( $record->user_name );
576586 $link = $record->af_user_id ? "User:$name" : "Special:Contributions/$name";
 587+ $gender = $name;
577588
578589 return Html::openElement( 'h3', array(
579590 'class' => $class
580591 ) )
581592 . Linker::link( Title::newFromText( $link ), $name )
582593 . Html::element( 'span', array( 'class' => 'icon' ) )
583 - . Html::element( 'span',
584 - array( 'class' => 'result' ),
585 - wfMessage( $message, $gender, $extra )->escaped()
 594+ . Html::element( 'span',
 595+ array( 'class' => 'result' ),
 596+ wfMessage( $message, $gender, $extra )
586597 )
587598 . Html::closeElement( 'h3' );
588599 }
Index: trunk/extensions/ArticleFeedbackv5/SpecialArticleFeedbackv5.php
——@@ -18,7 +18,7 @@
1919 private $filters = array(
2020 'comment',
2121 'helpful',
22 - 'all'
 22+ 'visible'
2323 );
2424 private $sorts = array(
2525 'age',
——@@ -41,8 +41,9 @@
4242
4343 if( $wgUser->isAllowed( 'aftv5-see-hidden-feedback' ) ) {
4444 array_push( $this->filters,
45 - 'invisible', 'unhelpful', 'abusive', 'needsoversight'
 45+ 'unhelpful', 'abusive', 'invisible'
4646 );
 47+ # removing the 'needsoversight' filter, per Fabrice
4748 }
4849
4950 if( $wgUser->isAllowed( 'aftv5-see-deleted-feedback' ) ) {
Index: trunk/extensions/ArticleFeedbackv5/ArticleFeedbackv5.hooks.php
——@@ -175,9 +175,6 @@
176176 'articlefeedbackv5-undelete-saved',
177177 'articlefeedbackv5-oversight-saved',
178178 'articlefeedbackv5-unoversight-saved',
179 - 'articlefeedbackv5-comment-link',
180 - 'articlefeedbackv5-special-sort-asc',
181 - 'articlefeedbackv5-special-sort-desc',
182179 'articlefeedbackv5-comment-more',
183180 'articlefeedbackv5-comment-less'
184181 ),

Follow-up revisions

RevisionCommit summaryAuthorDate
r110070ATF5 - fix issues from r109258 (not calling text() on messages) - one of the ...gregchiasson19:56, 26 January 2012
r110071r110068: Update ignore messagesraymond20:02, 26 January 2012
r110132Fixes issues raised in commits from last night:...gregchiasson15:57, 27 January 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r108881AFTv5 Special page work - get rid of the COUNT and OFFSET queries via an addi...gregchiasson23:52, 13 January 2012
r109254AFT5 - short circuit gender thing for the time being, so we can test the layo...gregchiasson00:06, 18 January 2012

Comments

#Comment by Nikerabbit (talk | contribs)   07:27, 27 January 2012 
+		. Html::element( 'span', 
+			array( 'class' => 'result' ), 
+			wfMessage( $message, $gender, $extra )
 		)

This is still double escaping, needs ->text().

#Comment by Santhosh.thottingal (talk | contribs)   06:06, 30 January 2012

To make the code review easier it would be better if you can do split the commits across bugs, functional changes, etc. A single commit with multiple functional changes, message changes, UI changes, code clean up, bug fixes etc are difficult to review.

Status & tagging log