User:Catrope/ArticleAssessment

From MediaWiki.org
Jump to: navigation, search

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

Things I need to know in order to fix all issues[edit]

  • 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[edit]

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[edit]

  • 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[edit]

  • 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[edit]

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

ArticleAssessmentPilot.sql[edit]

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

ApiListArticleAssessment.php[edit]

  • 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[edit]

  • 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[edit]

  • 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[edit]

  • Use jQuery manipulation rather than {PLACEHOLDERS} r72608
  • 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 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[edit]

  • No RTL support; is this needed?