User:Catrope/ArticleAssessment

ArticleAssessmentPilot review as of r72524 (Tue Sep 7, 10:05 UTC)

Things I need to know in order to fix all issues

 * Would it be OK for the interface to display "This article has been revised more than 5 times" rather than the exact number of revisions? That would allow us to remove a potentially evil database query
 * Well-Sourced is an adjective (and uses title case, bad) whereas the other ratings are nouns (Brandon question?)
 * Do you currently intend to deploy this anywhere else than on English Wikipedia?
 * Do you currently care about any of the following?
 * Making AAP work in RTL
 * Making  work
 * For English vs. other languages
 * Making AAP work in IE6 and other older browsers vs. just turning itself off in these browsers

Full review
This lists all issues I found, both things I can fix myself as well as things I need more information for (also listed above).

General things

 * Does this extension rely on any recent-ish (as in not currently deployed) trunk revs?
 * Version param in OutputPage::addScriptFile is not in 1.16wmf4 but that can be taken care of
 * Needs testing to confirm 1.16wmf4 compatibility
 * PARAM_REQUIRED missing
 * Document the fact that this extension only works in NS_MAIN somewhere
 * $wgArticleAssessmentRatingCount should be replaced with an array of currently used rating IDs (current code assumes this is a continuous range from 1 to RatingCount, not the case if ratings are removed and others are added later)
 * I saw Erik test this in IE6 the other day and it kinda exploded. We need to fix breakage in older browsers, blacklist incompatible browsers, or a combination of the two.
 * Shouldn't appear on history, old revs, or edit page

ArticleAssessmentPilot.php

 * Document settings a bit more clearly, especially $wgArticleAssessmentStaleCount and $wgArticleAssessmentCategory
 * Document what happens when the latter is set to empty (which it is by default).
 * Credit Adam Miller, who I assume wrote the front end

ArticleAssessmentPilotHooks.php

 * isInCategory: does SELECT 1 FROM table WHERE conds; work reliably cross-DBMS? Maybe selecting a real field is safer
 * addScript, addMessages are unused
 * addScript, addMessages are unused

ArticleAssessmentPilot.sql

 * Document tables and fields better. Much better - ?
 * article_assessment_pages comment suggests per-revision storage, but it's per-page

ApiListArticleAssessment.php

 * Should be called ApiQueryArticleAssessment.php per established precedent
 * Top comment empty
 * Should use $wgArticleAssessmentRatingCount rather than hardcoding $limit * 4
 * Omitting pageid parameter results in evil query (full table scan without &aauserrating, filesort with it). Suggest making pageid mandatory, see also below
 * revid may only be used in conjunction with userrating, so checking for isset( $params['revid'] ) || $params['userrating'] is unnecessary and leads to errors if revid is set but userrating isn't
 * Staleness detection code is broken, should use the revid of the last rated row rather than the revid of the last processed row.
 * All this also seems to assume there's only one page ID, in which case that param should be mandatory
 * The COUNT query scans a potentially unbounded number of rows. If you're really just trying to detect more than $wgAAStaleCount revisions are newer than your revid but don't care exactly how many, just use something like SELECT rev_id FROM revision WHERE rev_page=$pageID AND rev_id > $lastRev LIMIT $staleCount+1 and check how many rows it returns. /
 * It seems you do display the number in the interface and added a caveat should the counts be disabled. Maybe use "revised more than $1 times"?
 * No query-continue, so the module seems to not support multi-page mode anyway. But then why bother with the limit param?
 * revid param needs type:int

ApiArticleAssessment.php

 * action= modules usually don't have a param prefix
 * Should use $wgArticleAssessmentRatingCount rather than hardcoding LIMIT 4
 * "Fold for loop into foreach above?" --> Yes
 * Roan removed this TODO?
 * Use multi-row INSERT query instead of inserting each rating separately
 * Use false as a default value for $lastRating rather than 0, which could be inserted as a rating (although perhaps not with this UI)
 * insertPageRating and insertUserRating have @param docs but no general function docs
 * Use $dbw->timestamp for inserting timestamps into the DB
 * "Don't do this if the insert was successful" -> use $dbw->affectedRows
 * Couldn't find in DatabaseBase when looked for before! Reedy 12:03, 7 September 2010 (UTC)
 * aa_page_id is unneeded in UPDATE WHERE because of PRIMARY KEY
 * Set mustBePosted

ArticleAssessmentPilot.i18n.php

 * Well-Sourced is an adjective (and uses title case, bad) whereas the other words are nouns (Brandon question?)
 * PLURAL for articleassessment-stalemessage-revisioncount (config var could be set to 1)
 * articleassessment-stalemessage-norevisioncount is unused per the FIXME

ArticleAssessment.js

 * Use jQuery manipulation rather than {PLACEHOLDERS}
 * Use wrapping and jQuery manipulation rather than |Magic and regex evilness
 * Use $output.find( '.article-assessment-show-ratings a' ).click( function { ... } ) rather than duplicating code
 * .data( 'articleAssessment-context' ).config; --> use defensive programming
 * success: function( data ) { foo( data ); } --> just use success: foo
 * Use defensive programming for data.query.articleassessment
 * for ( rating in ... ) --> use var
 * Don't shadow rating with var rating inside the loop
 * Use numerical, not textual indices for hidden inputs, that's so much less of a pain
 * And avoids hardcoding the names of the ratings, eww
 * Avoid calling .click to call hooked functions, define them and call them directly instead
 * $1 expansion breaks down in all sorts of edge cases (multiple occurrences of the same var, messages with 10+ arguments)
 * At least the former should be fixed, the latter is not an issue for this extension
 * No client-side PLURAL implementation, so PLURAL doesn't work
 * Seems to work because you're getting preprocessed messages from wfMsg but really doesn't: always assumes zero case
 * Not a big deal for English but bad for other languages
 * Is this supposed to be rolled out on wikis other than enwiki anyway?
 * Don't use for .. in on args array in getMsg

ArticleAssessment.css

 * No RTL support; is this needed?