User:Catrope/Extension review/ArticleFeedbackv5
For initial review based on r105164 see archive.
These are the outstanding issues as of r105928. See also the fixme list.
ArticleFeedbackv5.php
edit
Done Line 82, 103: please be aware that tracking bucketing generates a lot of noise in the tracking data, to the point where it brought the site down once. This can only be enabled on the cluster if we use UDP logging (which we want to do anyway, I just need to get around to enabling it), and I will probably want to have it turned off initially when deploying.Fixed in r105309- I couldn't tell from the wording whether this meant the tracking was dangerous (which seemed more likely), or the bucketing was dangerous, so I averted both. The bucket config is now set to ignore = 100 by default, and ext.articleFeedbackv5.js skips bucketing if either track or ignore is set to 100. -- rsterbin
- I meant the tracking, i.e. generating an event for 'I just put user X in bucket Y' and logging all of those events. I'm sorry for not being clearer; all I asked for was to remove
'tracked' => true,throughout (which has not happened, so my concerns are not addressed with r105309)- Thanks for clarifying -- that makes a lot more sense than my first interpretation. Fixed in r106019 -- rsterbin
- I meant the tracking, i.e. generating an event for 'I just put user X in bucket Y' and logging all of those events. I'm sorry for not being clearer; all I asked for was to remove
- I couldn't tell from the wording whether this meant the tracking was dangerous (which seemed more likely), or the bucketing was dangerous, so I averted both. The bucket config is now set to ignore = 100 by default, and ext.articleFeedbackv5.js skips bucketing if either track or ignore is set to 100. -- rsterbin
- Line 161: are you guys even using the survey stuff anymore? If not, it should be stripped out of v5
SpecialArticleFeedback5.php
editOverall it looks like this special page is very much unfinished, and it is undeployable in its current state. This either needs serious work before deployment, or needs to be disabled by commenting it out from $wgSpecialPages.
api/ApiViewFeedbackArticleFeedbackv5.php
edit- Line 89: a COUNT(*) query on an unbounded number of rows is unacceptable in production. Doesn't the rollup table have this information? Sort of, but that would break the counts when a filter is active - hidden, visible, etc.
- Line 123: don't use OFFSET-based paging. Instead, page with WHERE af_page_id >= $params['continue'] and set a query-continue for the next page ID. Grep the core code for query-continue for examples, or ask me
- Line 201, 204: use the 'pipe-separator' What does this mean? (gregchiasson)
- It means use
wfMessage( 'pipe-separator' )to get the pipe symbol. That comment should have been "use the 'pipe-separator' message", sorry about that.
- It means use