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

Admins delete under multiple rationale, use template-generated delete reasons #300

Merged
merged 6 commits into from Jan 15, 2016

Conversation

Projects
None yet
2 participants
@MusikAnimal
Collaborator

MusikAnimal commented Nov 14, 2015

This is incomplete. Still needs testing, and there's at least one bug I haven't been able to fix

  • Allow admins to delete under multiple rationale
  • Allow admins to use the subgroups to enter in values such as the URL for G12 deletions
  • Use the corresponding template to generate a deletion summary
  • Move custom rationale to top with it's own section, and supply the rationale generated by the CSD template already on the page, if it exists
@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 14, 2015

@atlight When there's already a CSD tag on the page, the module has "custom rationale" selected with the template-generated rationale in the subgroup input field. Problem is I can't get that subgroup to hide when I click on another radio button. If I go back and forth twice it will work as it should.

Basically I'm having trouble understanding what hides the subgroups. I put a breakpoint in openSubgroupHandler within twinklespeedyGenerateCsdList, and it never gets hit, no matter what I click on.

userMultipleRadioClick: 4, // check boxes, subgroups, need to add a "Submit" button
userSingleSubmit: 5, // radio buttons, subgroups, submit when "Submit" button is clicked
userSingleRadioClick: 6, // radio buttons, subgroups, submit when a radio button is clicked
sysopMultipleSubmit: 2, // check boxes, subgroups, "Submit" button already present

This comment has been minimized.

@atlight

atlight Nov 16, 2015

Collaborator

The mode constants were very carefully laid out in an odd-even pattern to satisfy the code at line 247. Please test the code again with the "When to go ahead and tag/delete the page" Twinkle preference set to "As soon as I click an option" (i.e. speedySelectionStyle = radioClick) and fix the code as required.

I also wonder whether there need to be separate single/multiple sysop modes. The reason why tagging has separate single and multiple modes is that some CSD templates (e.g. db-hoax and the A7 subcategories) are not properly supported by db-multiple, so we have to prevent a different list to the user. (Arguably they could be combined into one, but that's a separate issue for another day.) For actual page deletion, is there any reason why we couldn't just get rid of the radio-button list and always show checkboxes?

return Morebits.userIsInGroup('sysop') && ($("#delete-reason").length ? decodeURIComponent($("#delete-reason").text()).replace(/\+/g, ' ') : '');
};
Twinkle.speedy.customRational = [

This comment has been minimized.

@atlight

atlight Nov 16, 2015

Collaborator

Misspelt.

@@ -248,6 +270,10 @@ Twinkle.speedy.callback.modeChanged = function twinklespeedyCallbackModeChanged(
var radioOrCheckbox = (Twinkle.speedy.mode.isMultiple(mode) ? 'checkbox' : 'radio');
work_area.append( { type: 'header', label: 'Custom rationale' } );
work_area.append( { type: radioOrCheckbox, name: 'csd', list: Twinkle.speedy.generateCsdList(Twinkle.speedy.customRational, mode) } );

This comment has been minimized.

@atlight

atlight Nov 16, 2015

Collaborator

Doesn't need to be under a separate header for non-admins. For tagging, simply merge the customRationale array with the generalList array and add them to the general header, to preserve current behaviour. Otherwise you are just making the list longer for no good reason.

return;
}
var normalized = Twinkle.speedy.normalizeHash[ value ];
var normalizeds = [];
$.each(values, function(index, val) {

This comment has been minimized.

@atlight

atlight Nov 16, 2015

Collaborator

Can just use values.map here, I think.

// returns => [<string> wikitext, <object> newParams]
var buildData = Twinkle.speedy.callbacks.getTemplateCodeAndParams(params),
code = buildData[0];
params = buildData[1];

This comment has been minimized.

@atlight

atlight Nov 16, 2015

Collaborator

Since it's only modifying params.utparams within the params object, just make it return [wikitext, utparams]. Better encapsulation that way, I think.

};
var api = new Morebits.wiki.api( 'Fetching deletion summary from template', query, callback);
api.post({
async: false

This comment has been minimized.

@atlight

atlight Nov 16, 2015

Collaborator

Yikes! Don't use non-async queries.

This comment has been minimized.

@MusikAnimal

MusikAnimal Nov 16, 2015

Collaborator

Yeah, I wouldn't normally do async either but I saw it was being done here on L1647. It's just so tough to work without deferred/promises! I'll do a series of callbacks instead

userMultipleSubmit: 5, // check boxes, subgroups, "Submit" button already pressent
userMultipleRadioClick: 6, // check boxes, subgroups, need to add a "Submit" button
userSingleSubmit: 7, // radio buttons, subgroups, submit when "Submit" button is clicked
userSingleRadioClick: 8, // radio buttons, subgroups, submit when a radio button is clicked

This comment has been minimized.

@MusikAnimal

MusikAnimal Nov 17, 2015

Collaborator

@atlight Fixed this bug. As for having only multi-select for admins: I can see admins (myself included) wanting to pick a specific A7 criteria. Secondly I think it would be an odd user experience to have the "custom rationale" as a checkbox, that would have to be disabled should you check other criteria

// are we in "delete page" mode?
// (sysops can access both "delete page" [sysop] and "tag page only" [user] modes)
isSysop: function twinklespeedyModeIsSysop(mode) {
return mode === Twinkle.speedy.mode.sysopSubmit ||
return mode === Twinkle.speedy.mode.sysopSingleSubmit ||
mode === Twinkle.speedy.mode.sysopMultipleSubmit ||
mode === Twinkle.speedy.mode.sysopRadioClick;

This comment has been minimized.

@atlight

atlight Nov 17, 2015

Collaborator

Four sysop modes now..?

This comment has been minimized.

@MusikAnimal

MusikAnimal Nov 17, 2015

Collaborator

to match the four user modes, right? similar scenarios, just need to delete instead of tag

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 17, 2015

Two outstanding bugs, as far as I can tell:

  1. As described above, when there's already a CSD tag, I can't get the custom rationale subgroup (input field) to hide when another radio button is selected.
  2. When in sysopMultipleRadioClick mode with speedySelectionStyle as radio, the module deletes as soon as a rationale checkbox is selected, rather than allowing you to select multiple ones then a hit submit under "When finished choosing criteria, click"

Any insight on these two issues? Thank you very much for your help and time in reviewing this!

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 17, 2015

@atlight just making sure you saw my note on the two outstanding bugs :)

@atlight

This comment has been minimized.

Collaborator

atlight commented Nov 18, 2015

I'll have a look...

@atlight

This comment has been minimized.

Collaborator

atlight commented Nov 18, 2015

For number 1, try adding event({ target: subnode }); after line 388 in morebits.js. See if that fixes it.

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 18, 2015

Deleting last two comments... fixed bug number 1! Now for number 2, which is the bigger issue

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 18, 2015

@atlight Alright! Going to move forward with testing in production, with the help our friend Callanecc :)

Let me know if you see any issues code-wise. Thanks!

speedy: Allow admins to delete under multiple rationale, using templa…
…te-generated deletion summaries

twinkle: changing of defaults - delete talk page, no confirmation of deletion summary for any criterion, open talk pages in new tab rather than new window, which is blocked by most browser by default
@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 24, 2015

@atlight Made some changes to the default Twinkle preferences. First, don't prompt for deletion summary under any criterion. This is not needed so much anymore with the advent of being able to pick multiple ratione and supply params, etc. Delete talk page by default, which we almost always want to do (except for user talk pages, which are excluded).

Finally, open user talk pages in a new tab and not a window. This is unrelated to the speedy update, but I figure this has been long overdue. Most browsers will block popups, so you have to explicitly allow Twinkle to do it. Most people prefer tabs anyway, I am willing to bet. Any input on this change is appreciated, otherwise I'm ready to merge in the speedy update. I've got a number of admins waiting for it! :)

label: 'Open user talk page on submit',
value: 'openusertalk',
name: 'openusertalk',
tooltip: 'Use this is override your preferences for the selected rationale',

This comment has been minimized.

@atlight

atlight Nov 25, 2015

Collaborator

Word "is" doesn't make sense here.

Also, if I were an admin, I would have trouble figuring out how this checkbox interacts with my preferences. This could do with some more explanation in the tooltip.

This comment has been minimized.

@atlight

atlight Nov 25, 2015

Collaborator

Also, this never seems to get checked in the case of multiple criteria

This comment has been minimized.

@MusikAnimal

MusikAnimal Nov 25, 2015

Collaborator

You could loop through the selected rationale and see if your preferences are all the same for those criteria, but I don't really think this is that advantageous. In the case of multiple criteria we'll just let the admin decide what to do. Maybe it should default to checked, though

This comment has been minimized.

@MusikAnimal

MusikAnimal Nov 25, 2015

Collaborator

Correction, it leaves the checkbox state as-is once you choose to delete under multiple rationale. I feel this is best, as I don't want to make any assumptions or go about the crazy logic I mentioned above.

@atlight

This comment has been minimized.

Collaborator

atlight commented Nov 25, 2015

I tried deleting a page under G4, typing garbage characters into the subgroup textbox. I was notified that my input was invalid (which is true), but then there was a JavaScript error: TypeError: params.templateParams is null and the CSD dialog sat there in an unusable state.

};
var statusIndicator = new Morebits.status( 'Building deletion summary' );
var api = new Morebits.wiki.api( 'Fetching parsing deletion template', query, function(apiObj) {

This comment has been minimized.

@atlight

atlight Nov 25, 2015

Collaborator

This isn't grammatical.

This comment has been minimized.

@MusikAnimal

MusikAnimal Nov 25, 2015

Collaborator

Haha, this and the comment above about is are a result of working on this during wee hours of the night

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 25, 2015

Fixed the bug you found, and made some copy edits. Not sure about the "open user talk page" when deleting under multiple criteria. I don't want to default to checked as some will find it annoying. If it gets unchecked when they didn't want it to they can view the deleted edits and click on the user talk page link there. I don't think we'll get many complaints.

@atlight

This comment has been minimized.

Collaborator

atlight commented Nov 25, 2015

I'll just test under "radioClick" and other modes, and then am happy to merge this.

The checkboxes and controls at the top of the list for admins are starting to get a bit out of hand. It would be helpful if, rather than disabling irrelevant controls (i.e. the "tag-related options" when in delete mode, and vice versa), the irrelevant checkboxes were actually hidden altogether. This would save vertical real estate. I won't insist on it for this change though; in fact, it would probably be better to address this in a separate pull request just to keep this PR manageable.

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 25, 2015

Yes I was thinking the same. I was even considering making it checkbox-based like the block module. Probably easier said than done.

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 25, 2015

^ @atlight one more commit. Figured it's better to at least say a deletion summary couldn't be generated by the template rather than assume the admin didn't enter one. In production, it will most certainly be the latter and not some bug with the templates, but nonetheless good to be informative.

@atlight

This comment has been minimized.

Collaborator

atlight commented Nov 25, 2015

A bug: Use Twinkle as a non-sysop in radioClick mode. Open CSD and click "G6: Housekeeping". You are never prompted for a rationale.

I am really wondering whether it is worthwhile to keep supporting radioClick. Only 18 users use it... But there are 9 admins among these 18 (and also 2 socks!), and I imagine some of them* would be a bit irate if we got rid of it. If it is not too much effort, we should try to keep supporting radioClick, but I can see the code could easily get out of hand.

* The admins, not the socks

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Nov 25, 2015

@atlight Alright, fixed I believe. I can try to reach out to those admins in particular and ask how dependent they are on the radioClick mode.

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Dec 2, 2015

@atlight sorry to bug you again... how do we feel about this?

@atlight

This comment has been minimized.

Collaborator

atlight commented Dec 2, 2015

I tested it out on a non-admin account, but pretending to be a sysop. The deletion encountered an error (obviously), but the error message immediately disappeared, being replaced by "Opening user talk page edit form for This, that and the other: complete". Although this specific situation is contrived, it is a problem if deletion error messages aren't hanging around to let people read and copy them.

@atlight

This comment has been minimized.

Collaborator

atlight commented Dec 2, 2015

I tried deleting as a sysop under the G7 criterion, but the rationale I entered into the form disappeared into the ether...

@atlight

This comment has been minimized.

Collaborator

atlight commented Dec 2, 2015

Also, a bunch of the form inputs aren't used to construct the deletion summary when deleting with multiple criteria. I think you should make more liberal use of the hideWhenMultiple and hideWhenSysop options in the subgroup configuration. Maybe we even need a hideWhenSysopMultiple option for a select few?

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Dec 7, 2015

@atlight fixed the last 2 bugs you mentioned, I believe (some of those subgroups didn't do anything when requesting speedy in multi-mode). How do I pretend to be a sysop?

@atlight

This comment has been minimized.

Collaborator

atlight commented Dec 9, 2015

How do I pretend to be a sysop?

You are a sysop, so you don't need to pretend; I, on the other hand, am not a sysop, so I run wgUserGroups.push('sysop') in the browser console so Twinkle thinks I am one.

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Dec 22, 2015

FYI if you're going through and reviewing stuff, this one still needs a little work. I've just been sidetracked with other matters.

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Jan 6, 2016

@atlight Just now getting back to this... I tried deleting as a non-admin but got Deleting page: Failed to delete the page: Permission denied when I submitted. Are you able to reproduce?

Also, just a general question, how do you normally do development/testing of Twinkle (or any MediaWiki JavaScript project)? I noticed you had very little Twinkle related-edits on testwiki. Are you using another wiki or just pasting in the code in the JS console on enwiki?

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Jan 6, 2016

Actually maybe I fixed that bug with this. Were there any other outstanding bugs you encountered?

I've been using it regularly on enwiki without issues. In a future update I'd like to clean up the interface some as you suggest. I also had the idea that maybe we could make post-delete templates to put on user talk pages, and automate adding those. I found Template:Nn-warn-deletion and others, which is what admins would add after they've deleted a page (e.g. if user didn't issue a CSD nomination template). I am convinced many admins don't know these even exist. I will have to set it up so that we have "delete" / "add delete template to user talk page" checkboxes, just like we do on the Block module.

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Jan 15, 2016

@atlight Going to merge this if there is no opposition. I've been using it for a while without issues, and will be sure to swiftly address any concerns users bring up once it's rolled out. Many thanks

@atlight

This comment has been minimized.

Collaborator

atlight commented Jan 15, 2016

Feel free to merge it, but make sure you're around to fix any reported bugs :)

@MusikAnimal

This comment has been minimized.

Collaborator

MusikAnimal commented Jan 15, 2016

Will do. Going to be cautious and deploy this tomorrow morning (EST) when I'm going to be in front of a computer all day.

Also... still curious how you do Twinkle development. I didn't know if you knew of an easier way? I write code in my text editor, copy/paste into the relevant MediaWiki namespace file on testwiki, reload whatever page, and do testing. This is tedious. Copy/pasting into the JS console would be initially quicker, but with a refresh I'd have to copy/paste again. Chrome has some sort of "work off of filesystem" feature that maybe I should look into.

MusikAnimal added a commit that referenced this pull request Jan 15, 2016

Merge pull request #300 from MusikAnimal/speedy-admins
Admins delete under multiple rationale, use template-generated delete reasons

@MusikAnimal MusikAnimal merged commit 554664b into azatoth:master Jan 15, 2016

@MusikAnimal MusikAnimal deleted the MusikAnimal:speedy-admins branch May 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment