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

Add support for change tags, remove summaryAd #969

Merged
merged 2 commits into from
Aug 14, 2020

Conversation

Amorymeltzer
Copy link
Collaborator

See individual commits for details, but basically:

  • Defines Twinkle.changeTags = 'twinkle' in twinkle.js and applies that across the board
    • Applied to Morebits.wiki.page via setChangesTags(), and manually to Morebits.wiki.api via tags
  • Excludes PageTriage (T252980) and FlaggedRevs (T247721) (and other
  • Remove customization of summaryAd preferences (they options remain hidden as backups, including for use in the above)

There's some precedence for at least being polite and asking before creating a tag like this; I'll post at VPT at some point. cc @MusikAnimal since this would be a notable change. Closes #265

@Amorymeltzer Amorymeltzer added Feature request Module: morebits The morebits.js library Module: twinkle The twinkle.js global gadget file Module: config deprecations Functions, etc. from Morebits that are deprecated and should be checked for usage before removal labels Jun 13, 2020
@Amorymeltzer
Copy link
Collaborator Author

Amorymeltzer commented Jun 18, 2020

This could be nicer to situations where no tag is desired, thus summaryAd would be. I'm open to ideas for handling that nicely. One way I suppose would be to change setChangeTags to adjust ctx.editSummary if given a second parameter, so that it'd look something like:

this.setChangeTags = function(tags, summary) {
	if (!summary) {
		ctx.changeTags = (!!tags && (typeof tags === 'string' ? tags : tags.join('|'))) || '';
	} else {
		ctx.editSummary = ctx.editSummary + summary;
	}
};

pageobj.setChangeTags(Twinkle.changeTags);
pageobj.setChangeTags(null, Twinkle.getPref('summaryAd'));

That's really not ideal. It'd also require having setChangeTags after setEditSummary every time.

A quick survey (grep -n -B 1 setChangeTags *|grep -v setChangeTags |grep -v setEditSummary|sort|uniq) suggests only a few items would need to change, namely the places in things like tag and xfd where setChangeTags is used before the callback so that it applies down the line, although that's a nice behavior.

@Amorymeltzer Amorymeltzer force-pushed the twinkletags branch 3 times, most recently from 4a251d4 to 6f3ab6d Compare June 30, 2020 03:07
@Amorymeltzer Amorymeltzer marked this pull request as ready for review June 30, 2020 03:08
@Amorymeltzer Amorymeltzer added the Other wikis non-EnWiki issues and language/i18n/l10n stuff label Jul 8, 2020
@Amorymeltzer
Copy link
Collaborator Author

@Xi-Plus IIRC you already have a tag, but I wonder how other folks (@Huji?) feel about the above idea re: adding appending to the edit summary if no tag is defined: #969 (comment)

@MusikAnimal
Copy link
Collaborator

Just now seeing this. Exciting! Change tags are much easier to query than edit summaries :) Especially because users can alter or blank the summaryAd, making it difficult or impossible to identify when Twinkle was used.

Change tags don't apply to logged actions like deletion, right? I think we should continue to use summaryAd on those. You might consider removing the ability to configure it too, if people don't care. The point is to be able to identify semi-automation, and if we allow the "ad" to be configurable then we could lose that.

@Amorymeltzer
Copy link
Collaborator Author

@MusikAnimal Thanks, and they do! You've basically identified all the reasons for it, but inclusion for actions and in log entries is another big one. Noted in the commits, but, for things we care about, only PageTriage and FlaggedRevs don't allow them yet.

I do kind of like the idea of hardcoding it, even beyond presence in prefs.

@Amorymeltzer
Copy link
Collaborator Author

Amorymeltzer commented Jul 26, 2020

Having thought about it for a bit, I don't think we should try to auto-edit edit summaries. It's a lot of extra overhead and using a changetag simplifies and improves a lot of things greatly.

Rebased, etc. I've incorporated @MusikAnimal's suggestion above, the two remaining uses of Twinkle.getPref('summaryAd') (FlaggedRevs and PageTriage, which don't currently allow for revision tags) now use the hardcoded Twinkle.summaryAd (for emotional continuity, although I'd prefer clarity taglessAd). The deprecated summaryAd, deletionSummaryAd, and protectionSummaryAd remain in Twinkle.defaultConfig but not in twinkleconfig.js, so they should be available for anyone relying on them for a bit (atm, I think just Belle's arb.js) but not customizable.

Any final objections or suggestions? I'll be notifying WT:TW/WP:VPT tomorrow.

@Amorymeltzer
Copy link
Collaborator Author

Leaving morebits tag blank per feedback, but the option remains.

@Amorymeltzer Amorymeltzer force-pushed the twinkletags branch 3 times, most recently from 4fbdd91 to d54f751 Compare August 8, 2020 21:16
… of summaryAd

Defines `Twinkle.changeTags = 'twinkle'` in twinkle.js and applies that across the board.

There are a lot of actions, only some of which support applying tags.  For a full list, search for `ApiBase::PARAM_TYPE => 'tags'` (in core; for extensions use something like that e.g. `::PARAM_TYPE => 'tags'`).  The fullish list is:

* block
* changecontentmodel
* delete
* edit
* import
* managetags
* move
* patrol
* protect
* revisiondelete
* rollback
* setlanguagepage
* tag
* unblock
* undelete
* upload
* userrights

Basically the majority of the "write modules."  Others that support `tags`: globaluserrights, the Wiki Love extension, and various WikiBase APIs; notable exceptions for our purposes are PageTriage (T252980) and FlaggedRevs (T247721).  This implementation weeds out the most common/likely for our purposes: checking against the full supported list is unwieldy, and largely unnecessary as the error message is harmless.  If we did want to check everything, we'd just dump that list into an object and verify the relevant action.

This also deprecates the `summaryAd` preferences.  `summaryAd`, `deletionSummaryAd`, and `protectionSummaryAd` have been retained in twinkle.js' `defaultConfig` as backups when a tag isn't used (e.g. add-on scripts, installations that don't want to use a tag), but this also formally removes them as preferences to be selected from `twinkleconfig.js`.  `deletionSummaryAd` and `protectionSummaryAd` should be removed entirely IMO, in favor of just one "ad" that could itself be better integrated.

----

At first blush it would seem to be simpler to just do something like we do for the useragent, that is:

```
Morebits.wiki.api.setActionTag = function(tag) {
	morebitsWikiActionTag = ...
};
```

But doing that would mean the tag would be applied to any actions made using the Twinkle structure, such as addons like https://en.wikipedia.org/wiki/User:Bellezzasolo/Scripts/arb.js.  Moreover, given the likely coincidence between 1. scripts using the morebits library, and 2. users of Twinkle, that would effectively make this a "morebits" tag rather than "twinkle."  Having the invocation on a per module basis rather than `twinkle.js` only solves the issue if one and only one action is opened and then successfully performed, which is likewise unreliable.  Instead, the structure used here means setting the tag manually and individually, using either the `tags` parameter for `Morebits.wiki.api` actions or `.setChangesTags` for `Morebits.wiki.page` (or by setting `this.changeTags` for `Morebits.userspacelogger`, done to avoid adding another parameter).

Morebits (the library) maintains its ability to set its own tag for non-twinkle actions, although that might be undesirable and is turned off.
Fairly rote.  Added `summaryAd` to twinkleprotect's `stabilize`, since that was missing it (T247721); didn't do the same for `triage` since while `pagetriageaction` has a `note` parameter that functions like an edit summary we could use, it felt like overkill to add something to morebits just so we can modify `ctx.note` for these largely pointless log entries.  Ideally, https://phabricator.wikimedia.org/T252980 will be taken care of first.  There are a few instances where `setChangeTags` is used earlier than might otherwise be expected, so as to apply to downstream options; a number of these are the `triage` calls, which are thus, as noted, nonfunctional.
@Amorymeltzer Amorymeltzer merged commit 7478ed2 into wikimedia-gadgets:master Aug 14, 2020
@Amorymeltzer Amorymeltzer deleted the twinkletags branch August 14, 2020 10:36
@Amorymeltzer
Copy link
Collaborator Author

🎉 🏷️ 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Functions, etc. from Morebits that are deprecated and should be checked for usage before removal Feature request Module: config Module: morebits The morebits.js library Module: twinkle The twinkle.js global gadget file Other wikis non-EnWiki issues and language/i18n/l10n stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use tag instead of edit summary suffix to identify Twinkle edits
2 participants