User:Catrope/Extension review/MarkAsHelpful

• Is addMarkAsHelpful() supposed to contain more code in the future or something?
• You don't have to export the edit token to a JS variable anymore now that 1.19 is deployed. You can use mw.user.tokens.get( 'editToken' ) in JS instead.
• The API unmark action doesn't use a unique key to obtain the object it unmarks. You should use the ID to unmark. Alternatively, you could make the type,item,user_id,user_ip combination a unique index, that would allow you to get rid of the PHP code (in MarkAsHelpfulItem::userHasMarked()) that prevents duplicate submissions. Preventing duplicates must be enforced on the DB level, doing this on the PHP level will always fail in race conditions
• You should be passing error codes or i18n message keys from the API, rather than i18n-ed messages. The latter works most of the time but breaks down when things like &uselang= are in play.
• User::newFromName( $this->getProperty( 'mah_user_ip' ) ); doesn't work, because User::newFromName() returns false for IP inputs.
• unmark() function
  • You appear to be using the 'type' check to check whether the data is loaded. If that's what you're doing, add an isLoaded() function, or add a comment indicating what you're doing. The code is confusing because 'type' is obtained then never used.
  • The id==id || name==name logic is broken. Two anonymous users with different IPs will pass the isAnon==isAnon check, and they'll also pass id==id (both 0), so any anon can unmark any other anon's helpful-thing, even if their IP addresses differ. This is obviously bad, but it's not clear that it should be allowed even if the IPs do match, because IPs change or be shared. Rob said that IPs weren't allowed to submit helpful-things, but I don't see that in the code, and if that's the case there shouldn't be any code to deal with anons to begin with.
  • markAsHelpfulList() should have a LIMIT to prevent the query from fetching an unbounded number of rows
• JS code
  • selector: '[class^=markashelpful-]', //only selector for now --> it would be way nicer to just put a common class on everything, or something. If you're using attribute regexes on the 'class' attribute, it's very likely that you're doing something wrong
    • aha, I see you're doing this because you're encoding data in the class names. Don't do this. Use attributes like data-markashelpful-item and data-markashelpful-type instead, and fetch the data with $foo.data( 'markashelpful-item' )
  • //only inject once per item id to prevent copy & paste of hook in pages --> I don't see what kind of duplication could possibly be prevented here. Surely the PHP side is smart enough not to output the same thing twice? Ironically, it looks like this guard prevents the refresh code in the success callback of the POST AJAX request from working

You should probably also look at my commits to this extension.