"Catrope" changed the status of MediaWiki.r111472 to "fixme" and commented it.
URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/111472#c31698
Old Status: new
New Status: fixme
Commit summary for MediaWiki.r111472:
bug 34090 - Added an additional column in the main feedback table to keep a
count of the number of activities which have happened for each piece of
feedback - this should be equivalent to running a "count(*)" on the logging
table for a specific piece of feedback. Made sure the counter is incremented
when a new log entry concerning activity is added. Created a new api to get an
html block of activity for use on the feedback page. Returns a json object
with "activity" = html block of activities, more set to true IF there are more
entries to fetch, can have the limit changed and uses continue to get the next
entries if more is true, send it the feedbackid and optionally a limit and/or
continue value
Catrope's comment:
<pre>
+ af_activity_count integer unsigned NOT NULL DEFAULT 0
</pre>
<pre>
+ if( !$wgUser->isAllowed( 'aftv5-hide-feedback' )) {
+ return;
+ }
</pre>
You should return an error message rather than just returning nothing. Use
<code>$this->dieUsage( "You don't have permission to hide feedback",
'permissiondenied' );</code> or something.
<pre>
+ $page = Title::newFromID( $feedback->af_page_id );
+ $title = $page->getPartialURL();
</pre>
This will throw a fatal error if someone supplies a bad page ID, because
Title::newFromID() can return null. This is in addition to what Benny said
about $feedback possibly being false. And this also needs ->getText() rather
than ->getPartialURL(), just like the other instance we talked about earlier.
Note that ->getText() will return a string with spaces rather than underscores,
so for use in DB queries against log_title you will need to use ->getDBKey()
instead. You can also use ->getDBKey() throughout if you want, the Title
constructors accept both text form and dbkey form (but not URL form).
<pre>
+ // <div>Feedback Post #{$feedbackid} by {$user_link}</div>
+ $html .= Html::openElement( 'div', array() );
+ $html .= wfMessage( 'articlefeedbackv5-activity-feedback-info',
+ array($feedback->af_id))->text()
+ . $this->getUserLink($feedback->af_user_id, $feedback->af_user_ip);
+ $html .= Html::closeElement( 'div' );
</pre>
This is what we call lego messages, and should be avoided. String concatenation
is not a valid replacement for $1 parameters. Instead, you should add a $2
parameter to the message that takes the user link. Because it's HTML, you'll
need to use ->rawParams() rather than ->params() to set it.
<pre>
+ array( $feedback->af_activity_count ))->text() );
</pre>
This needs number formatting, i.e. <code>array( $wgLang->formatNum(
$feedback->af_activity_count ) )</code> . Also,
<pre>
+ 'articlefeedbackv5-activity-count' => '$1 actions on this post',
</pre>
needs pluralization, i.e. <code>{{PLURAL:$1|$1 action|$1 actions}} on this
post</code> . This doesn't seem terribly important in English, but other
language (either Polish or Russian, I forget) have plural cases that resemble
numbering in English (-st, -nd, -rd and -th) so it definitely matters for those.
<pre>
+ $html .= $this->getUserLink($item->log_user, $item->log_user_text)
+ . Html::element( 'div', array(),
+ wfMessage( 'articlefeedbackv5-activity-' . $item->log_action,
+ array())->text() )
+ . $wgLang->timeanddate( $item->log_timestamp );
</pre>
This is also lego.
<pre>
+ if (!empty($item->log_comment)) {
</pre>
[[Manual:Coding_conventions#PHP_pitfalls|Don't use empty()]], it suppresses all
errors, so if someone misspelled log_comment here no one would ever find out.
Perhaps you meant <code>!$item->log_comment</code> or <code>$item->log_comment
== ''</code> or something else.
<pre>
+ // figure out if we have more based on our new continue value
+ $more = $this->fetchHasMore($title, $feedbackId, $continue);
</pre>
The preferred way to do paging in MediaWiki is to fetch one row more than you
need (i.e. <code>'LIMIT' => $limit + 1</code>) and then have a counter that
ensures that excess row is never output, but its ID is used as the continue
value. This is different from what you're doing, you're outputting the ID of
the last row as the continue value and then checking separately if there are
more results. With my suggested approach, you would have to use <code>foo >=
$continue</code> rather than <code>foo > $continue</code> , but you wouldn't
have to check separately if there are more results (presence or absence of the
excess row indicates this) and you wouldn't have to output this fact separately
either (presence or absence of the continue parameter indicates this).
Reading the actual query, it looks like you are requesting
<code>$limit+1</code> rows, but the code that processes these rows is not aware
of it? I don't quite get it.
<pre>
+ $result->addValue( $this->getModuleName(), 'continue', $continue );
</pre>
The standard way of doing this is <code>$this->setContinueEnumParameter(
'continue', $continue );</code> . That will put it in the query-continue
element.
<pre>
+ $where[] = 'log_id < ' . intval($continue);
[...]
+ 'ORDER BY' => 'log_timestamp DESC'
</pre>
These don't mix well together (read: the database really will not like this
query). You'll need to do your ordering and your continuation using the same
field or set of fields. I think you can probably get away with ordering by
log_id instead of log_timestamp, because out-of-order entries should be very
rare and will probably only be off by one or two seconds if at all (and
presumably your timestamp display in the UI shows timestamps to the minute, not
to the second).
<pre>
+ parent::__construct( $query, $moduleName, 'af' );
</pre>
Module prefixes should be unique, and I'm pretty sure <code>'af'</code> is
already in use by another AFTv5 module.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
