User:Bawolff/review/Newsletter

T110170

This is review of Newsletter extension for conformance with MediaWiki standards, performance, and security. I'm not reviewing anything to do with User requirements, or architecture.

I've including things here that are suggestions to consider as well as things that are more critical. Stuff that's really important is marked (important).

Most of my comments are pretty brief, and don't really explain why or how. If you're confused by any of them, don't understand them, or have any other questions, don't hesitate to ask.

General impressions:
 * The code follows mediawiki coding conventions, and is generally easy to read :)

Newsletter.php

 * My understanding is that this is a complete rewrite from the code Siebrand wrote. In that case the  * @copyright Copyright © 2013 Siebrand Mazeland should be changed.
 * There is no extension.json file.

SQL

 * Generally MediaWiki code uses column names that start with a short prefix of the table name, even for foreign keys in the table, which newsletter does not do. However, I don't think this is a hard rule for extensions (?)
 * The table defintions use REFERENCES keyword. Most of the MediaWiki mysql schema avoids foreign key constraints. I believe this is intentional, although I'm not sure the exact reason. You should double check with someone about using them.
 * (important) nl_newsletters needs a secondary index on nl_name

onEchoGetDefaultNotifiedUsers

 * (??? come back to this)Batching queries to user table
 * How does this function scale. Will there ever be 100000 subscribers?

modules
'jquery.cookie', 'jquery.tabIndex', 'mediawiki.jqueryMsg', 'mediawiki.api', 'jquery.confirmable' As far as I can tell, only mediawiki.api is needed.
 * console.log( data ). I think this should be mw.log. Also, i think this could be an error on old browsers without native console.
 * You list a lot of dependencies:

api

 * The modules end in the name api, which seems superflourus.
 * No validation.
 * Is it limitted to logged in users (Don't want user id 0 in your tables)
 * post
 * CSRF

Special pages
It would be nice (Although possibly not critical) if all these worked without js. Th

SpecialNewsletters.php

 * No submit button. Most users don't expect that adjusting a radio button would cause a subscription action without some other indication of success (Although the subscriber count does help indicate that it does). I think a submit button would be more inline with what users expect. However, this is perhaps something that it would be good to get wmf design team input on (Or whatever the equiv is post-reorg).
 * It would also be nice if newsletters you are subscribed to were highlighted somewhere (E.g. the table row becomes light blue or something)
 * I find putting the subscriber count in a disabled text box kind of odd. Why not just a normal table cell?
 * (important) Lots of static fields. For most of these, I think a private field would make more sense. Especially things like $subscribedNewsletterId, which depends on the specific instance of the object (As it depends on the user it was constructed with), and has the potential (albeit a very low potential given how MW usually works, but the lifetime of SpecialPage objects in a normal MW request is not something you should rely on) of becoming out of sync.
 * (important) getSubscribedNewsletters, should be a public function, not a static one. It should probably simply return its data, instead of storing it in member fields of itself

$res = $dbr->select(                                       'nl_newsletters',                                        array( 'nl_main_page_id' ),                                        array( 'nl_name' => $value ),                                        __METHOD__                                );
 * Two things about the previous code block:
 * Provided you're querying the total subscribers for all newsletters, it should be done as a single query, e.g. SELECT newsletter_id, count(*) from nl_subscriptions group by newsletter_id; (The separate queries might make more sense if you had a LIMIT clause to not do a full count of subscribers)
 * This has poor scalability. It requires a full table scan. This is only ok if the sum of the subscribers to all newsletters is small (Say less than 5000)
 * Doing it this way, doesn't take into account paging. You have to get the subscriber count of all newsletters, even if you're only displaying a subset of newsletters. It would be better to do this queries from the Pager code.

$mainPageId = ''; foreach( $res as $row ) { $mainPageId = $row->nl_main_page_id; } return ''. $value. ''; in_array( $this->mCurrentRow->nl_id, SpecialNewsletters::$allSubscribedNewsletterId ) ? SpecialNewsletters::$subscriberCount[$this->mCurrentRow->nl_id] : 0, function isFieldSortable( $field ) { return false; }
 * Two things for this block of code:
 * If you're only interested in a single field, use $dbr->selectField instead.
 * Queries like this should really be batched, or better yet, done directly from getQueryInfo</tt> using a join
 * Use Linker class instead. I would also suggest in the case where the main page id isn't a valid page, to simply not link instead of linking to the current page.
 * in_array is somewhat inefficient on large arrays. Not critical, but it would be better to do isset( SpecialNewsletters::$subscriberCount[$this->mCurrentRow->nl_id] ) ? SpecialNewsletters::$subscriberCount[$this->mCurrentRow->nl_id] : 0</tt>
 * (important) line 157, 147: $this->msg( 'newsletter-subscribe-button-label' );</tt> This should have a ->parse on the end. Additionally, you may want to consider using the &lt;label&gt; html element here.
 * Seems like nl_name should be sortable (So people can go in reverse order).
 * Not critical, as its mostly understandable from context, but it would be nice if each function had code comment blocks with @param...