Implementing blocking tools for admins

[MusikAnimal]

Hi, I'm a big JS programmer (my profession, actually), and admin on enwiki. I wrote spamublock.js which is great for what it is, but there's tons of other common blocks I make daily, like vandalism-only and NOTHERE, etc.

So I got to thinking. Instead of writing a new script, design and implementing a GUI from scratch, why don't I try to add this functionality to Twinkle? I've talked with some admins on #wikipedia-en-admins and they agreed an addition like this would be a long time coming. Any thoughts? I'm willing to put a lot of work into this and make this happen. Thanks!

[atlight]

This would be great. It would be possible to create a new "block" module, which would include the existing block warning functionality from the "warn" module as well as functionality to actually block users using API action=block. I think the "protect" module could be a good model, although you would have to get rid of all the RPP stuff.

I would imagine, similar to "protect", that it would be possible via this new module to simply post a block warning in cases where the block was carried out manually (similar to the "tag only" option in "protect"), or to actually carry out the block (posting a warning being optional).

I should also note, if you wish to proceed with this, that we don't use mw.api. Instead, we use our own wrapper Morebits.wiki.api (and Morebits.wiki.page for page-based actions like editing). In particular, we do not use JSON, instead relying on the XML response format from the API and using jQuery to select relevant content from the response.

In any case, I would be happy to help with development and review, although my time is somewhat limited.

[MusikAnimal]

Oh dear, no JSON. I can settle without, I suppose! Is there a particular reason for that? Just curious!

I have no idea how much work this is going to be. I was planning on just going off of the page protection module as you say. Blocks would work much the same way, where you have presets for all the templated blocks (those prefixed with uw-), each with their standard block options which you can change in the interface. We may also have the optgroups to separate by good-faith and bad-faith, such {{uw-spamublock}} vs {{uw-softerblock}}. And maybe eventually do like I did with spamublock.js and have it perform the deletion of the relevant page as well.

I'll start trying to find my way around the codebase and let you know when I've started working on it. Thanks for being so responsive... I'll surely have more questions for you. Where's the best place to talk? I don't like posting my email on here in the off-chance the recruiters are reading, but from enwiki you can use Special:Email/MusikAnimal. Cheers

[atlight]

Just quickly, re JSON, as far as I know we use mw.Api + JSON in two places (protect and arv). In protect it is used because we don't really care if it fails (IIRC). The usage in ARV wasn't done by me and really ought to be replaced.

The reason why we use XML is probably a historical one: Twinkle was written back in 2007, and XML was probably the only format the MediaWiki API offered at that point. As for why you shouldn't use mw.Api, it isn't integrated with Twinkle's error handling and completion tracking systems. Our custom wrappers, Morebits.wiki.api and Morebits.wiki.page, are tightly integrated with Status and Morebits.wiki.actionCompleted (or whatever it's called).

[Technical-13]

@MusikAnimal This is what my endgame plan for spamublock was when I said "more". I'd be happy to help whenever I have a free moment. :)

[MusikAnimal]

Cool. I could probably move through this fairly quickly if I can just find a good way to test what I'm doing. I've read the README, but it's assuming everything you're doing is in the MediaWiki space. Is there not an easy way to just work out of my userspace? I'm working on testwiki, so should I just use the MediaWiki-namespaced files there and enable the test gadget? Is there a way I can make the sync script go to there instead of enwiki? Thanks

[MusikAnimal]

I have the interface more or less completed. I've opened a pull request but it's not actually a request yet, just wanted to open it so you guys could comment on it should you want to. Thanks for any input, PR at https://github.com/azatoth/twinkle/pull/260/files You can see what I've got on testwiki, just go to any userpage and type Twinkle.block() in the JS console, then select Block from the menu. Again this is just the interface, submitting the form won't do anything.

[MusikAnimal]

Okay, two weeks and a couple hundred testwiki edits later, we have a working block module! In summary, it can block, reblock, and issue block templates on the user's talk page. Unblocking is not yet supported, but I plan to get to that. Other features are in store as well, but let's focus on what we've got.

The data for block templates was moved from the warn module to the block module, but the warn module is used to actually issue the warning, and also previewing the templates. I also cleaned up some of the code in the warn module, most of it solely for block module integration.

The templates were also grouped together, although differing from MediaWiki:Ipbreason-dropdown. We could go back and forth about grouping, but I think the way I've put it makes it very easy to find what you want, the options within each group sorted alphabetically. The "common block reasons" are what's truly the most common, as in the ones admins use every day. I hope others agree with the sorting, in practice I think this will be a very welcomed change.

To summarize changes to the existing warn module: ~ Block templates moved to block module, grouped by popularity / type and sorted alphabetically ~ Some of the defaults for the block templates were changed. For instance, {{uw-spoablock}} or "sockpuppetry" does not enforce an indefinite expiry, as we usually don't indef the sockmaster. ~ Added more block templates

I'm surely missing lots of things, and obviously there's lots of testing in store here. I guess let me know anything I can do to improve on the technical side / Twinkle conventions, then when we're ready I could post at [[WP:VPT]] asking for testers/feedback.

Pull request: https://github.com/azatoth/twinkle/pull/260

Block module is enabled by default for sysops using dev version of Twinkle on testwiki. I've been blocking User:Example. I'm not sure about IPs, I can only assume there's a some safe IP range we can use for testing.

[atlight]

I'll leave a detailed reply soon, but if you want to test blocking IPs, it's safe to block anywhere between 192.0.2.0 and 192.0.2.255 (or the whole range).

[MusikAnimal]

I've addressed most concerns brought up. I'm happy to use Morebits.wiki.api if preferred. I see some examples to go off of, but is there any official documentation for all the Morebits stuff? Will it print any errors, as with Morebits.status.warn?

[atlight]

Don't stress too much about uploading additional commits - I'll be continuing to leave comments over the next few hours. Unfortunately GitHub, unlike Gerrit, doesn't allow you to draft all your comments and then save them in one hit.

Morebits.wiki.api has quite complete error handling.

[atlight]

I have left a number of inline comments at the pull request. Some of them are general broadly-specific remarks that I probably should have left here instead.

As for documentation of Morebits.wiki.api, there is a little bit in morebits.js itself. To find examples, you could simply grep the repository; off the top of my head, it's used in batch undelete and probably other places.

Although I prefer XML to JSON for most purposes, the use of XML in new code like this is a little anachronistic. If you're interested, you could create something like Morebits.wiki.apiJSON that would be a copy-paste of Morebits.wiki.api with some of the back-compat stuff removed, the interface (including constructor parameters) cleaned up a bit, and conversion of XML to JSON. (I'd rather not extend the existing api object, as its interface has gained various accretions over the years and isn't as attractive as it once was.) However, that's totally optional, and I'm not unhappy with a mix of mw.Api for non-critical JSON fetches and Morebits.wiki.api for critical-path XML fetches.

[MusikAnimal]

Alrighty, I've addressed all concerns brought up in the code review. I'm using Morebits.wiki.api for the block, it was easy to use as we aren't doing anything with response in that case. However I am still using mw.Api to fetch the token, and for the initial API call to fetch the user data. Other changes unrelated to the code review include several bug fixes and some much needed refactoring.

As for integrating with the Warn module, I think overall it's not that convoluted. The only truly nasty bit is that 10–15 line data transformation, and if we wanted to rid of that the better solution in my opinion would be to rework how Warn shows the options so that it utilizes Morebits helpers. That would make the other templates in Warn easier to manage and pave the way to sort by optgroup if we wanted to (which we probably should, but that's for a different pull request). Along with the template data, the essential shared functionality is issuing the warning itself. Here we have considerable logic involved, checking Twinkle preferences, making sure the warning goes under the right heading, date parsing and formatting, etc.

Let me know what you think of the updated pull request. If all checks out I'd like to explore what we may want to do for user configuration, then get some testers on board as there may be more bugs I have yet to find. Thanks for thorough review!

[atlight]

I'm going to play around with this a bit, so bear with me.

[atlight]

I still have significant reservations about code sharing between warn and block. It's going to make the code in warn more difficult to maintain. Most of us usually don't have a whole lot of time for maintaining Twinkle, and I don't really want to have to be testing both warn and block modules when changing something in warn.

So let me see what functionality in warn you are using from block:

• Twinkle.warn.callbacks.preview. This is a trivial function and half of it is a special case for blocking anyway. Just create a block-specific version of this function under Twinkle.block and get rid of the block special-casing from warn.
• Twinkle.warn.callback.change_subcategory. This is another function that is almost entirely special-cased for block. Again, just create a block-specific version of this function under Twinkle.block and get rid of the block special-casing from warn.
• Twinkle.warn.callbacks.main. So what does this do?
  • Checks that no warning has been issued in the last one minute. I don't think this is relevant for blocking, as if you've just blocked the user, you are going to want to template them regardless of any other templates that may have been posted to them. And in any case I don't think it works very well, as server time and user clock time are not always going to be perfectly synchronised.
  • Some logic to do with date headers. I didn't realise we used date headers for blocking templates - I assumed we used something like "== Blocked ==" for these. I suppose this is something we might want to share, but it can be refactored out into a simple function under Twinkle.warn if wanted.
  • The shared IP notice. I don't think this is relevant for blocking templates. I would hope all the IP block templates include information about shared IPs within the context of the template.
  • Actually apply the template. Nothing special here.

I really think it would be a lot simpler to untangle the weird hidden fields, data transformations, and other hacks and just put everything in Twinkle.block, except possibly for the date header business.

Let me know what you think. As I said, I'm really reluctant to merge this with the interaction between block and warn as it is, because I can foresee it causing headaches in the future.

[atlight]

I also notice you've made extensive modifications to Twinkle.warn.callback.change_category. I can't for the life of me tell why you've done this, as this function doesn't seem to actually be called from the block module at all...

[atlight]

Oh I see what's going on... you're making the block templates available from both warn and block. That explains why you're resorting to all these hacks!

I would say, just make them accessible from one place (namely block); admins will just have to get used to the change.

[MusikAnimal]

Alrighty, pull request updated. Moving block-only stuff over to Block is very much sensible. I guess I didn't realize the extent of how much block-specific stuff there was, much less how easy it was to just cut and paste into Block! I believe there was also a fundamental misunderstanding from when I first started this project that we did want to share functionality between the two modules, but I'm going to blame that on myself.

I've addressed all concerns brought up I believe. The logic for date headers could be shared, but we're only saving ourselves a few lines of code. I'll leave it to you whether we want to bother with that. Thanks for explaining all the Warn functions in detail. I have regularly been using the block module on enwiki (again existing only in my userspace), and I can attest that it has become indispensable. I look forward to getting this out and making the admin land a little more happy for all.

[atlight]

Done :)