Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

batchdelete: allow deletion of subpages #581

Merged
merged 3 commits into from
May 30, 2019

Conversation

siddharthvp
Copy link
Member

Trying to solve #305. Though I can't be sure whether this works without having the sysop bit.

This handles the subpages just as the redirects are being handled (they are deleted without being listed, so individual pages can't be de-selected). We may need to think about whether sysops ought to be deleting subpages without knowing anything about which are the subpages or how many of them are there (until they see the "Deleting ___" status messages). If deemed necessary, this can be tweaked to list out all the subpages along with the main pages (so that they are known to the user beforehand, and can be de-selected).

@Amorymeltzer
Copy link
Collaborator

Haha great minds: I'm on a bus with limited wifi, and figured this was an easy thing to whip up while watching Firefly. This is nearly exactly what I wrote up — I've got a couple longer comments I'll make when I get a chance, but overall structure is fine.

@JJMC89
Copy link
Contributor

JJMC89 commented Apr 12, 2019

This shouldn't be done in namespaces where subpages are not enabled, like ns:0 on enwiki.

I think it would be better to have a selection like d-batch.

Example case:

  • Article to be deleted: Foo
  • Another article at Foo/bar should not be deleted.
  • Talk:Foo and Talk:Foo/Archive n should be deleted but not Talk:Foo/bar and Talk:Foo/bar/Archive n.

Similar things occur with templates where a template name contains a / but isn't part of and shouldn't be deleted when the root template is.

@Amorymeltzer
Copy link
Collaborator

Amorymeltzer commented Apr 13, 2019

That basically covers what I was going to say, namely this ''needs'' to ignore Mainspace. I don't quite follow on templatespace, though — @JJMC89 do you have an example? I don't think there's a js variable exposed for this config, I can open a phab (though it won't help here).

Anyway, the other major difference between this and my code was I tack on a delete_talkpages to each subpage; that is, if we're deleting subpages of User:DouglasAdams, then User:DouglasAdams/42 as well as User talk:DouglasAdams/42 should be deleted. Incidentally, we don't do this for any talkpages of redirects, but probably should if do it for subpages (let's wait until we've figured this out).

That's straightforward enough, but the tricky issue is subpages of talk pages without corresponding pages, aka archives. I basically dealt with this by not dealing with it. I think we should include some notes/tooltips, but basically they're currently not being deleted anyway so it's no loss. Moreover, I don't know that we want to automatically remove all talk page archives, and I think it'd be better to have sysops have to explicitly note the existence of talk pages in their list. How's that sound @JJMC89?


As for listing the individual subpages, what this code will do is briefly list the items as they are deleted, and then once deleted will collapse that to a single line saying something like "Deleting subpages of page (7/7)". That is obviously most efficient, and is similar to how we handle redirects in that there's no confirmation/etc., but I do share the concerns that there could be a lot of subpages deleted without knowing it.

Basically, I'm worried that this is basically enabling the -r in rm -rf, and we all know how that can end. It hopefully shouldn't come up too much, and it's literally the point of doing this, but we need to account for the fact someone could fuck up and delete every AfD. I think the three obvious options are:

  • Only enable subpages for certain namespaces (easy)
  • Require manual confirmation for every list of subpages for every page (medium)
  • Dump them all in a new dbatch list and delete that list (hard)

@JJMC89
Copy link
Contributor

JJMC89 commented Apr 13, 2019

I don't quite follow on templatespace, though — @JJMC89 do you have an example?

I couldn't find the original example that I was thinking of, but Template:Yes/no (redirect to Template:Yesno) isn't related to Template:Yes.

That's straightforward enough, but the tricky issue is subpages of talk pages without corresponding pages, aka archives. I basically dealt with this by not dealing with it. I think we should include some notes/tooltips, but basically they're currently not being deleted anyway so it's no loss. Moreover, I don't know that we want to automatically remove all talk page archives, and I think it'd be better to have sysops have to explicitly note the existence of talk pages in their list. How's that sound @JJMC89?

I'm not really too concerned with talk archives. In most cases, pages being deleted don't have them.

I think the three obvious options are:

  • Only enable subpages for certain namespaces (easy)
  • Require manual confirmation for every list of subpages for every page (medium)
  • Dump them all in a new dbatch list and delete that list (hard)

I don't think deleting all subpages without confirming what is being deleted is a good idea, and I would prefer to be able to select which subpages to delete.

@siddharthvp
Copy link
Member Author

Dump them all in a new dbatch list and delete that list (hard)

I think this option is the way to go. I spent a couple of hours on this and hope to fix it up soon. I don't think there is a way to list out all the subpages of n different pages using a single API call; it requires n API calls. So @atlight's Morebits.batchOperation needs to be used. The trouble with the batchOperation framework is that it does not give an option to provide a callback ( which we need to list the pages on the interface) that executes after the whole batch has processed. I guess I'll have to add that ability to the batchOperation class.

@Amorymeltzer
Copy link
Collaborator

Amorymeltzer commented Apr 15, 2019

@JJMC89 I see now — the problem being, of course, that the software thinks [[Template:Yes/no]] is a subpage of [[Template:Yes]]. I think templatespace would be one of the man places this would be desirable, with so many /doc and other subpages frequently being left behind. Perhaps we'll have to rely upon sysops to be careful? I'm in favor of loading this up with a couple scary warnings.

ETA: There's no wg for namespace with subpages, but I knew I'd seen something before; we can access it via API. A query with meta=siteinfo and siprop=namespaces will let you know if subpages are enabled in that namespace. I think it's fine to hardcode-out mainspace to avoid the extra api call, but in the future we could check if query.[pageTitle.namespace].subpages exists.

@siddharthvp
Copy link
Member Author

@Amorymeltzer @JJMC89 Done. The list of all subpages will now be displayed below the main list.

Archives of talk pages won't be deleted. If we wish to delete them too, we'd have to query the list of subpages for all talk pages too, more than doubling the number of api calls.

I've provided additional options for i) deletion of redirects of subpages, and ii) unlinking of subpages, but not for deletion of talk pages of subpages (which is done if main talk page deletion is checked).

@Amorymeltzer
Copy link
Collaborator

Amorymeltzer commented Apr 16, 2019

I'm heading to bed so can't do a full review, but this will display subpages of every page, regardless of whether they're checked or not, right? That seems problematic. I don't think we want to regenerate the subpage list on the fly whenever one of the top-level page boxes is checked or unchecked, but the list of subpages should respect that list somehow, most likely when the subpages box is checked. Open to brainstorming. Form looks gorgeous, though.

@Amorymeltzer Amorymeltzer added the Module: batch The three batch modules and deprod label Apr 18, 2019
@MusikAnimal MusikAnimal force-pushed the master branch 2 times, most recently from 0fe4e21 to 88f94e7 Compare April 18, 2019 04:05
@siddharthvp siddharthvp force-pushed the batchdsubpages branch 2 times, most recently from f351c88 to 801120b Compare April 19, 2019 21:03
@siddharthvp
Copy link
Member Author

but this will display subpages of every page, regardless of whether they're checked or not, right?

Should have thought about that earlier. Fixed now.

Copy link
Collaborator

@Amorymeltzer Amorymeltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay @siddharthvp — I got busy, this got complicated, and it's a (comparatively) weird code structure I'm less familiar with. I'll try and tackle this this week.

modules/twinklebatchdelete.js Outdated Show resolved Hide resolved
modules/twinklebatchdelete.js Outdated Show resolved Hide resolved
@Amorymeltzer
Copy link
Collaborator

Amorymeltzer commented Apr 29, 2019

@siddharthvp I don't have the time/brainpower to delve into the code atm, but from a quick look and test, there's something weird happening when checking/unchecking boxes.

Test page has two pages linked, one mainspace and one user, the latter with subpages.

  1. Clicking dbatch opens the Twinkle menu with both checked (good)
  2. Clicking subpages loads subpages, all checked (good); unchecking subpages returns just the two pages, both checked (good). Toggling back and forth, they all remain checked (good)
  3. Unchecking or checking either page (either before or after toggling subpages) and then toggling subpages, they each unchecked (good)
  4. Subsequent re-checking of either page, however, is lost when subpages is toggled (bad)
  5. The above resets if the window is closed and reopened.

If I toggle a page on or off before toggling subpages, there's no issue, but it seems toggling the subpage box will retain a page's unchecked status if reached once the subpages menu is triggered.

@siddharthvp
Copy link
Member Author

Did you use the Select All/Deselect All buttons any time during the above tests? I belatedly realised that those buttons don't work properly in the presence of subpage checkboxes. I will fix that.

I also plan to add arrow > links next to the subpages (as we do in Tag module) for easy navigation to those pages. I too am very busy over the next 2 weeks, as you may have judged by my lack of response at the PRs where you've pinged me recently. I'll deal with this and the others whenever I get time.

@Amorymeltzer
Copy link
Collaborator

No, just the checkboxes. And no rush, thanks for letting me know.

Add an optional that runs after the batch has processed.
To be used in twinklebatchdelete.js
@siddharthvp
Copy link
Member Author

I've just pushed a new version, which is hopefully bug-free. Fixed the issue that you pointed out in bullet point 4 above. Fixed the select all/deselect all buttons. Fixed the issue with shiftclick support. Added > links to pages.

@siddharthvp
Copy link
Member Author

In order to fix the issue of shiftclick support to the main page list (when subpages are present), I have provided a fix in Morebits.checkboxShiftClickSupport, so that is the 3rd change to morebits in this PR. Basically, the issue was that when you try to uncheck multiple pages at once, though those pages do get unchecked, their lists of subpages didn't get hidden. (This was because checkbox.checked = true/false does not trigger the click event, whereas checkbox.click() does.

@siddharthvp
Copy link
Member Author

In the present scheme, the subpages are being deleted only after all the main pages (along with main pages' talkpages and redirects) have been deleted. It'd be better to delete subpages along with their parent page. But there seems to be no easy way of identifying which subpages belong to which page at evaulate-time.

@siddharthvp siddharthvp force-pushed the batchdsubpages branch 4 times, most recently from 3d31a32 to d856319 Compare May 16, 2019 03:42
@siddharthvp
Copy link
Member Author

note: I deleted the commit that added HTMLFormElement.prototype.getUnchecked, as it has already been added to morebits by the now-merged #485.

@siddharthvp
Copy link
Member Author

Have now tested this using the newly-gained admin bit on testwiki. Everything seems to work as intended.

Copy link
Collaborator

@Amorymeltzer Amorymeltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great code @siddharthvp, glad you ended up doing this since it turned out to be so involved! Only a few minor changes/questions needed here. One big issue though, that I haven't run down yet: I can't seem to get it to delete talk pages of subpages being deleted! Nope, I was in userspace, dumb move on my part. 🤕 Works like a charm!

morebits.js Outdated Show resolved Hide resolved
morebits.js Outdated Show resolved Hide resolved
morebits.js Outdated Show resolved Hide resolved
modules/twinklebatchdelete.js Outdated Show resolved Hide resolved
modules/twinklebatchdelete.js Outdated Show resolved Hide resolved
@Amorymeltzer
Copy link
Collaborator

My only other real nitpick is that if someone opens the dbatch menu, loads the subpage box, closes the dbatch menu, then re-opens it, the whole subpage function doesn't work. That's not something IMO that should block this, but I think it's a definite bug to deal with at some point.

@siddharthvp
Copy link
Member Author

My only other real nitpick is that if someone opens the dbatch menu, loads the subpage box, closes the dbatch menu, then re-opens it, the whole subpage function doesn't work.

Nice catch! Fixed. Will look at other comments when I have time.

@siddharthvp
Copy link
Member Author

Will look at other comments when I have time.

all resolved.

Rather than just marking the checkboxes as checked/unchecked, trigger the click event on the checkboxes, so that the click event listeners are activated in addtion to checkboxes being marked as checked/unchecked. While using quickForm for example, the event listener shows/hides the subgroup.
Lists of subpages for each page are listed on the interface, with checkboxes for subpages being in subgroupof the checkbox for main page. A link is provided for each page next to it.
@Amorymeltzer Amorymeltzer merged commit 2587c2f into wikimedia-gadgets:master May 30, 2019
@Amorymeltzer
Copy link
Collaborator

💯 work on this

Amorymeltzer added a commit to Amorymeltzer/twinkle that referenced this pull request Oct 27, 2019
… intentionally limited

- Actual code changes are two in `twinkledeprod`, one in `twinklespeedy`, and two in `twinklexfd` (similar work done in wikimedia-gadgets#568 and wikimedia-gadgets#581)
- Exceptions are four `rvlimit`s in `twinklearv` and one `rvlimit` in `twinklefluff`, all related to sequentially processing revisions.
- Unified comments:
    - `max`: `// 500 is max for normal users, 5000 for bots and sysops`
    - non-max: `// intentionally limited`
    - Removed erroneous comments for `batchMax` (holdover from [2007](https://en.wikipedia.org/w/index.php?title=User%3AAzaToth%2Ftwinklebatchdelete.js&diff=prev&oldid=169056183))
@siddharthvp siddharthvp deleted the batchdsubpages branch October 22, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Module: batch The three batch modules and deprod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants