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 {{PLURAL}} 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) r72540
• 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 r72530
  • Document what happens when the latter is set to empty (which it is by default). r72530
• Credit Adam Miller, who I assume wrote the front end r72530

ArticleAssessmentPilotHooks.php

• isInCategory(): does SELECT 1 FROM table WHERE conds; work reliably cross-DBMS? Maybe selecting a real field is safer
  • r72181#c8867
    • OK, never mind then
• addScript(), addMessages() are unused r72530

ArticleAssessmentPilot.sql

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

ApiListArticleAssessment.php

• Should be called ApiQueryArticleAssessment.php per established precedent r72528
• Top comment empty r72530
• Should use $wgArticleAssessmentRatingCount rather than hardcoding $limit * 4 r72532
• Omitting pageid parameter results in evil query (full table scan without &aauserrating, filesort with it). Suggest making pageid mandatory, r72534 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 r72534
• Staleness detection code is broken, should use the revid of the last rated row rather than the revid of the last processed row. r72538
  • All this also seems to assume there's only one page ID, in which case that param should be mandatory r72534
• 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. r72532/r72623
  • 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? r72532
• revid param needs type:int r72532

ApiArticleAssessment.php

• action= modules usually don't have a param prefix r72531
• Should use $wgArticleAssessmentRatingCount rather than hardcoding LIMIT 4 r72532
• "Fold for loop into foreach above?" --> Yes
  • Roan removed this TODO?
    • Actually, never mind, I was wrong
• Use multi-row INSERT query instead of inserting each rating separately was wrong about this one too
• Use false as a default value for $lastRating rather than 0, which could be inserted as a rating (although perhaps not with this UI) r72549
• insertPageRating() and insertUserRating() have @param docs but no general function docs r72532
• Use $dbw->timestamp() for inserting timestamps into the DB r72531
• "Don't do this if the insert was successful" -> use $dbw->affectedRows() r72537
  • 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 r72531

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} r72608
• Use <span> wrapping and jQuery manipulation rather than [[|Magic]] and '''regex''' ''evilness''
• Use $output.find( '.article-assessment-show-ratings a' ).click( function() { ... } ) rather than duplicating code my bad, no duplication
• .data( 'articleAssessment-context' ).config; --> use defensive programming
• success: function( data ) { foo( data ); } --> just use success: foo fixed
• Use defensive programming for data.query.articleassessment
• for ( rating in ... ) --> use var fixed
  • Don't shadow rating with var rating inside the loop fixed
• 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) fixed for multiple occurrences, good enough
  • At least the former should be fixed, the latter is not an issue for this extension fixed
• 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() fixed
• Use .data() instead of invalid original-title attribute

ArticleAssessment.css

• No RTL support; is this needed?