batchundelete: Improvements to morebits.wiki.page, short- and long-term alleviation of #613, color like batchdelete

[Amorymeltzer]

tl;dr — Add undeletePage to Morebits.wiki.page, use it in twinklebatchundelete (helps with #613), bump maxRetries (helps with #613), restructure talkpage undeletion (from #601), and add red color for fully create protected pages.

• 1st commit: morebits: Add undeletePage to morebits.wiki.page
  Basically just copy the stuff for deletePage, less following redirects. deletePage was added in 4b92869 along with move, but unlike protect (68132e5) delete and undelete are one-way streets. Uses a dumb kludge to get around the lack of a proper token until we can rewrite the token handling stuff in morebits (morebits: Modernize token handling #615).
• 2nd commit: batchundelete: convert to use Morebits.wiki.page.undeletePage
  Added in prior commit (47c9cf9). Some testing suggests this helps a bit with Batch undelete mostly unsuccessful for large numbers of undeletions #613, but regardless, it's clearer this way anyway.
• 3rd commit: batchundelete: temporarily bump maxRetries up to 3 to avoid DB errors and alleviate Batch undelete mostly unsuccessful for large numbers of undeletions #613
  Increase from 2 to 3, makes twinklebatchundelete much more likely to succeed until phabricator.wikimedia.org/T222402 can be dealt with. We've been handling internal_api_error_DBQueryError for deletion since morebits: while deleting, retry on DBQueryError #198, before even batchOperation was added (ecfff3b).
• 4th commit: batchundelete: Use same format in talkpage restoration as batchdelete
  Follows up on batchundelete: restore talk pages too #601. Allows for a better, more clear message when listing deletions, as well as more full error handling and clearer code. Easier to expand, should we desire.
• 5th commit: batchundelete: Add red color and confirmation for create-protected pages a la batchdelete

[Amorymeltzer]

cc @MusikAnimal I mentioned this at #601 (comment) to @siddharthvp but I'd like to merge this sooner rather than later to get deal with batchundelete. I did a bunch of testing and this all seems to work well, but it ended up being a fair bit of stuff. Regardless, I'll probably just bump maxRetries on-wiki later tonight/tomorrow when I have a moment, which should handle things temporarily anyway.

[siddharthvp]

This would unnecessarily slow down the code in batchundelete. Better to put this in fnProcessUndeleteError. No decently written script would show an undelete button to non-admins anyway.

This would apply to deletePage function also.

[Amorymeltzer]

Isn't this backwards? Failing here means non-sysop don't do the later protection query. At any rate, it's quite quick as is (although admittedly slowing things down isn't bad in our current situation).

[siddharthvp]

To clarify, this would slow down the code for sysops. Yes, it would speed up things a great deal for non-sysops but what good is that since they won't be able to undelete anything after all?

[Amorymeltzer]

I would guess the impetus was to allow for the "retrieving undelete token..." message?

[siddharthvp]

I don't understand, token = mw.user.tokens.get('editToken'); is being used whether or not this condition is true. Why have this condition at all?

[Amorymeltzer]

This is what I was trying to convey in the comment: until I can radically modernize the handling of tokens, I wanted to keep the code structure parallel. I think it's easier to understand and fix/remove/rewrite if as much as possible follows a similar, understandable pathway rather than a bespoke system for each. Yes, we can remove this fnCanUseMwUserToken, I suppose that'd be fine given the context of the comment.

[Amorymeltzer]

Thought I dealt with this, but that meant morebits failed, so I've undone it for now as I don't have any more time today.

[Amorymeltzer]

Oh duh! If suppressProtectWarning isn't set, the pages fail the "poor man's normalisation" in fnCanUseMwUserToken. suppressProtectWarning is set for the batch but not for talkpages so the kludge was only reached (in this case) for talk pages. This matches the behavior in twinklebatchdelete, and is required; since the batches check, display, and confirm on protection status for the pages listed (well, now batchundelete does) it's not needed for those, but the confirms(s) are needed for any talkpages found later. Sorry for leading you on @siddharthvp, I knew this a few days ago but Morebits gets confusing!

As far as fnCanUseMwUserToken goes, followRedirect doesn't really make sense for undeletePage so the suppressProtectWarning is the real killer. The real goal of that whole thing seems to be to avoid another API call to get the protection status if suppressProtectWarning isn't set, none of that is relevant for batch-processing like this. I'm inclined to leave it as is for now on the off chance we decide to use it elsewhere before revamping morebits, as well as, as mentioned elsewhere, making what will surely be an revamping process easier.

[siddharthvp]

I'm not sure if querying for deleted revisions of talk page, and then undeleting if they exist is a good idea, as this is a lot of extra API calls. What about doing it like this: store the list of pages whose undeletion was successful, in an array, and then use a separate batch operation to restore the talks. For this batch-op, unset the preserveIndividualStatusLines property, so that the status lines for these pages don't show up at all. We could also probably display just the number of talk pages undeleted.

How is this handled in batchdelete? Is a status line shown only for the main pages, or for talk pages and redirects too?

[Amorymeltzer]

batchdelete shows the status lines for each talk; I think it's helpful there (see also #581) and even moreso here, as undeleting talkpage is potentially unexpected. More to the point, a talkpage could exist and have deleted revisions, which might be a significant surprise. batchdelete does indeed do a query for each talkpage, to confirm that it exists and to quietly skip the page if it doesn't (rather than fail loudly).

As I'm writing this, I'm realizing that the check for missing in fnProcessUndelete is, as here, insufficient, and should be rewritten to include the deleted revision check.

ETA: Eh, I'm reconsidering this (the latter bit) in favor of the old way (only restoring if the page doesn't exist). Anything else might be too surprising and is beyond the scope of what I had initially attempted to do here.

---

I edited batchundelete to check for both missing and deleted revisions. The change is to only undelete if the page doesn't exist, but to only attempt if there are deleted revisions (avoiding a ton of errors, obviously).

[siddharthvp]

I edited batchundelete to check for both missing and deleted revisions.

Thanks, that makes a lot of sense and also makes my point moot, as my suggested method also would have ended up restoring deleted edits at existing talk pages.