r97969 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r97968‎ | r97969 | r97970 >
Date:22:45, 23 September 2011
Author:aaron
Status:resolved (Comments)
Tags:
Comment:
FU r97886:
* Check if setReviewingStatus() actually succeeds
* Optimized jQuery selecting a bit (engine is RTL)
* Removed highlightning of "please review" msg and tweaked wording
* Reduced JS code duplication
Modified paths:
  • /trunk/extensions/FlaggedRevs/presentation/RevisionReviewFormUI.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/language/RevisionReview.i18n.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/presentation/modules/review.js (modified) (history)

Diff [purge]

Index: trunk/extensions/FlaggedRevs/presentation/RevisionReviewFormUI.php
——@@ -144,29 +144,25 @@
145145 }
146146 $form .= "<p>";
147147 if ( $u !== null ) { // page under review...
148 - $form .= '<span class="fr-under-review">';
149148 if ( $u != $this->user->getName() ) { // by another user...
 149+ $form .= '<span class="fr-under-review">';
150150 $msg = $priorRevId
151151 ? 'revreview-poss-conflict-c'
152152 : 'revreview-poss-conflict-p';
153153 $form .= wfMsgExt( $msg, 'parseinline',
154154 $u, $wgLang->date( $ts, true ), $wgLang->time( $ts, true ) );
 155+ $form .= "</span>";
155156 } else { // by this user...
 157+ $form .= '<span id="mw-fr-reviewing-status">'; // JS widget
156158 $msg = $priorRevId
157159 ? 'revreview-adv-reviewing-c'
158160 : 'revreview-adv-reviewing-p';
159 - $form .= wfMsgHtml( $msg );
 161+ $form .= wfMsg( $msg ); // don't escape
 162+ $form .= "</span>";
160163 }
161 - $form .= "</span>";
162 - } else { // page not under review; add JS button
 164+ } else { // page not under review; add JS widget
163165 $form .= '<span id="mw-fr-reviewing-status" style="display:none;"></span>';
164166 }
165 - # Let user toggle advertising that they are reviewing this
166 - if ( $u === null || $u === $this->user->getName() ) {
167 - $form .= '<span id="mw-fr-reviewing-toggle" style="display:none;">';
168 - $form .= ' (<a href="javascript:void()"></a>)'; // JS activated
169 - $form .= '</span>';
170 - }
171167 $form .= "</p>\n";
172168
173169 if ( $disabled ) {
Index: trunk/extensions/FlaggedRevs/presentation/modules/review.js
——@@ -328,64 +328,47 @@
329329 'enableAjaxReviewActivity': function() {
330330 // User is already reviewing in another tab...
331331 if ( $('#mw-fr-user-reviewing').val() == 1 ) {
332 - var msgkey = $('#mw-fr-input-refid')
333 - ? 'revreview-adv-reviewing-c'
334 - : 'revreview-adv-reviewing-p';
335 - $('#mw-fr-reviewing-status').html(
336 - mw.msg( msgkey, [mw.config.get('wgUserName')] ) // advertised notice
337 - );
338 - $('#mw-fr-reviewing-toggle a').html(
339 - mw.msg('revreview-advertise-stop')
340 - ).click( FlaggedRevsReview.deadvertiseReviewing );
 332+ FlaggedRevsReview.isUserReviewing = 1;
 333+ FlaggedRevsReview.advertiseReviewing( null, true );
341334 // User is not already reviewing this....
342335 } else {
343 - var msgkey = $('#mw-fr-input-refid')
344 - ? 'revreview-sadv-reviewing-c'
345 - : 'revreview-sadv-reviewing-p';
346 - $('#mw-fr-reviewing-status').html(
347 - mw.msg( msgkey, [mw.config.get('wgUserName')] ) // suggest to advertise
348 - );
349 -
350 - $('#mw-fr-reviewing-toggle a').html(
351 - mw.msg('revreview-advertise-start')
352 - ).click( FlaggedRevsReview.advertiseReviewing );
 336+ FlaggedRevsReview.deadvertiseReviewing( null, true );
353337 }
354 - $('#mw-fr-reviewing-status').addClass('fr-under-review').show();
355 - $('#mw-fr-reviewing-toggle').show();
 338+ $('#mw-fr-reviewing-status').show();
356339 },
357340
358341 /*
359342 * Flag users as "now reviewing"
360343 */
361 - 'advertiseReviewing': function() {
362 - FlaggedRevsReview.setReviewingStatus( 1 );
 344+ 'advertiseReviewing': function( e, isInitial ) {
 345+ if ( isInitial !== true ) { // don't send if just setting up form
 346+ if ( !FlaggedRevsReview.setReviewingStatus( 1 ) ) {
 347+ return; // failed
 348+ }
 349+ }
 350+ // Update notice to say that user is advertising...
363351 var msgkey = $('#mw-fr-input-refid')
364352 ? 'revreview-adv-reviewing-c' // diff
365353 : 'revreview-adv-reviewing-p' // page
366 - $('#mw-fr-reviewing-status').html(
367 - mw.msg( msgkey, [mw.config.get('wgUserName')] )
368 - );
369 - // Invert toggle text/function...
370 - $('#mw-fr-reviewing-toggle a').html(
371 - mw.msg('revreview-advertise-stop')
372 - ).unbind('click').click( FlaggedRevsReview.deadvertiseReviewing );
 354+ $('#mw-fr-reviewing-status').html( mw.msg( msgkey, [mw.config.get('wgUserName')] ) );
 355+ $('#mw-fr-reviewing-stop').click( FlaggedRevsReview.deadvertiseReviewing );
373356 },
374357
375358 /*
376359 * Flag users as "no longer reviewing"
377360 */
378 - 'deadvertiseReviewing': function() {
379 - FlaggedRevsReview.setReviewingStatus( 0 );
 361+ 'deadvertiseReviewing': function( e, isInitial ) {
 362+ if ( isInitial !== true ) { // don't send if just setting up form
 363+ if ( !FlaggedRevsReview.setReviewingStatus( 0 ) ) {
 364+ return; // failed
 365+ }
 366+ }
 367+ // Update notice to say that user is not advertising...
380368 var msgkey = $('#mw-fr-input-refid')
381369 ? 'revreview-sadv-reviewing-c' // diff
382370 : 'revreview-sadv-reviewing-p' // page
383 - $('#mw-fr-reviewing-status').html(
384 - mw.msg( msgkey, [mw.config.get('wgUserName')] )
385 - );
386 - // Invert toggle text/function...
387 - $('#mw-fr-reviewing-toggle a').html(
388 - mw.msg('revreview-advertise-start')
389 - ).unbind('click').click( FlaggedRevsReview.advertiseReviewing );
 371+ $('#mw-fr-reviewing-status').html( mw.msg( msgkey, [mw.config.get('wgUserName')] ) );
 372+ $('#mw-fr-reviewing-start').click( FlaggedRevsReview.advertiseReviewing );
390373 },
391374
392375 /*
——@@ -412,8 +395,12 @@
413396 async : false
414397 });
415398 }
416 - FlaggedRevsReview.isUserReviewing = value;
417 - return ( call.status == 200 );
 399+ if ( call.status == 200 ) {
 400+ FlaggedRevsReview.isUserReviewing = value;
 401+ return true;
 402+ } else {
 403+ return false;
 404+ }
418405 }
419406 };
420407
Index: trunk/extensions/FlaggedRevs/presentation/language/RevisionReview.i18n.php
——@@ -65,10 +65,10 @@
6666 'revreview-successful2' => '\'\'\'Revision of [[:$1|$1]] successfully unflagged.\'\'\'',
6767 'revreview-poss-conflict-p' => '\'\'\'Warning: [[User:$1|$1]] started reviewing this page on $2 at $3.\'\'\'',
6868 'revreview-poss-conflict-c' => '\'\'\'Warning: [[User:$1|$1]] started reviewing these changes on $2 at $3.\'\'\'',
69 - 'revreview-adv-reviewing-p' => 'Notice: Other reviewers can see that you are reviewing this page.',
70 - 'revreview-adv-reviewing-c' => 'Notice: Other reviewers can see that you are reviewing these changes.',
71 - 'revreview-sadv-reviewing-p' => 'Notice: Please advertise yourself as reviewing this page.',
72 - 'revreview-sadv-reviewing-c' => 'Notice: Please advertise yourself as reviewing these changes.',
 69+ 'revreview-adv-reviewing-p' => '<span class="fr-under-review">Notice: Other reviewers can see that you are reviewing this page.</span> (<a id="mw-fr-reviewing-stop" href="javascript:void(0)">de-advertise</a>)',
 70+ 'revreview-adv-reviewing-c' => '<span class="fr-under-review">Notice: Other reviewers can see that you are reviewing these changes.</span> (<a id="mw-fr-reviewing-stop" href="javascript:void(0)">de-advertise</a>)',
 71+ 'revreview-sadv-reviewing-p' => 'You can <a id="mw-fr-reviewing-start" href="javascript:void(0)">advertise</a> yourself as reviewing this page to other users.',
 72+ 'revreview-sadv-reviewing-c' => 'You can <a id="mw-fr-reviewing-start" href="javascript:void(0)">advertise</a> yourself as reviewing these changes to other users.',
7373 'revreview-advertise-start' => 'Advertise',
7474 'revreview-advertise-stop' => 'De-advertise',
7575 'revreview-toolow' => '\'\'\'You must rate each of the attributes higher than "inadequate" in order for a revision to be considered reviewed.\'\'\'

Follow-up revisions

RevisionCommit summaryAuthorDate
r98807MFT r97886,r97892,r97899,r97969,r98179,r98654: ajax reviewing status code cha...aaron20:55, 3 October 2011
r100383REL1_18 MFT r97886, r97899, r97969, r98179, r98497, r98654, r98773, r98801, r...reedy21:36, 20 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r97886* Made "under review" notices require the user's consent (bug 31093)...aaron03:38, 23 September 2011

Comments

#Comment by Krinkle (talk | contribs)   20:08, 26 September 2011

mw.config.get('wgUserName') The result is the same, but it's semantically more correct to use mw.user.name(); Those weird non-config "config" variables should be deprecated at some point.

#Comment by Krinkle (talk | contribs)   20:32, 26 September 2011 
+	'revreview-adv-reviewing-p'    => '<span class="fr-under-review">Notice: Other reviewers can see that you are reviewing this page.</span> (<a id="mw-fr-reviewing-stop" href="javascript:void(0)">de-advertise</a>)',
+	'revreview-adv-reviewing-c'    => '<span class="fr-under-review">Notice: Other reviewers can see that you are reviewing these changes.</span> (<a id="mw-fr-reviewing-stop" href="javascript:void(0)">de-advertise</a>)',
+	'revreview-sadv-reviewing-p'   => 'You can <a id="mw-fr-reviewing-start" href="javascript:void(0)">advertise</a> yourself as reviewing this page to other users.',
+	'revreview-sadv-reviewing-c'   => 'You can <a id="mw-fr-reviewing-start" href="javascript:void(0)">advertise</a> yourself as reviewing these changes to other users.',
..
+		$('#mw-fr-reviewing-status').html( mw.msg( msgkey, [mw.config.get('wgUserName')] ) );
+		$('#mw-fr-reviewing-start').click( FlaggedRevsReview.advertiseReviewing );
 	},

Using ‎<a> and other html in messages is not something I think we should do. It has been done in a few places, but new instances should be avoided at all costs. Especially when including content that is not i18n-dependent (such as tagnames, ids and anchor targets). Suggesting to use something like this.

By the way, mw.msg takes variadic arguments. The array format isn't documented as far as I know.


I suggest something like this. Pinging Neil, since he may know a better way. 

	'revreview-sadv-reviewing-p'   => 'You can $1 yourself as reviewing this page to other users.',
	'revreview-advertise'   => 'advertise,
..
	var $underReview =  $(
		'<span class="fr-under-review">'
		+ mw.message( msgkey )
			.escaped()
			.replace(/$1/, mw.html.element('a', { id: 'mw-fr-reviewing-start' }, mw.msg( 'revreview-advertise' ) )
		+ '</span>'
	);
	$underReview.find( '#mw-fr-reviewing-start').click( FlaggedRevsReview.advertiseReviewing );
	$('#mw-fr-reviewing-status').empty().append( $underReview );

Status & tagging log