fluff: Only add links if the user has necessary perms to edit the page #632
Conversation
…ove redundant checks Better way of doing wikimedia-gadgets#630/e0f3d9c
Amorymeltzer
changed the title
|
@MusikAnimal, Pinging you for a a look since you had thoughts on #562 aka #511. Despite delaying loading of links on Special:Contributions, I think this'd be a good addition. If you think it'd be worth it, we could add a query check before loading on diffs/oldid to be more accurate than the software, but diffs and old revisions can take long enough to load and the edge cases (cascading, TitleBlacklist) are comparatively infrequent. cc @pppery |
Amorymeltzer
force-pushed
the
fluff-notifcantedit
branch
from
a51e69e
to
87b7153
Compare
Closes wikimedia-gadgets#605. Basically, we need a way to check for ability to edit the covers both explicit (protection) and implicit (namespace and content model) restrictions. Since we're on the page in question, `diff` and `oldid` (wikimedia-gadgets#562) can be handled together with mostly-accurate results by simply checking `wgIsProbablyEditable`. It won't handle cascading protection or TitleBlacklist restrictions, but neither does MediaWiki when choosing to display either "edit this page" or "view source"; being more accurate than the software would require an API check a la contributions (see following section as well as https://gerrit.wikimedia.org/r/c/mediawiki/core/+/65009). `contributions` is the more <s>complex</s> interesting implementation, since we've got to check each and every page. Querying for `edit` in `intestactions` is excellent, because it covers all bases (i.e., it includes the "expensive" checks that `wgIsProbablyEditable` skips, hence `Probably`). I also restructured/split the contributions routine to avoid doing `async` and to look more Twinkley. `intestactions` was added in MW 1.25 (https://www.mediawiki.org/wiki/MediaWiki_1.25) circa 2014-2015, and should probably be used more frequently by Twinkle, particularly in `Morebits`, as it's a more correct determiner of whether the user can edit (or move or protect or...) the page in question.
Amorymeltzer
force-pushed
the
fluff-notifcantedit
branch
from
87b7153
to
603d2b0
Compare
|
@Amorymeltzer I should probably have said earlier but I think this change was quite unnecessary. Can we revert this please? It slows down what I think is an already slow process of loading of rollback links on contribs pages. It would somewhat affect the work of RCP folks who are trying to revert vandalism fast. Also there is this obvious bug that no rollback links show up on pages with >50 current revs, as Easier option is to revert, as I don't see any major advantage out of this, that accounts for the additional delay in loading. Or if this is that important, a better way to do this would be to show all rollback links at first, then silently do the API call and remove the rollback links on non-editable pages. |
|
sigh All fair points.
This is what I was most worried about, but I don't think it's that much of a slowdown. My (admittedly inadequate) tests suggested it was noticeable but not significant, and split about halfway between the query and the
Yeah, no idea how I missed that. I tested on long contribs with my non-sysop alt, but I'm guessing my test users just didn't have enough unique pages edited at the time. This for me is a total dealbreaker, so I've partially reverted this (that is, just the contribs bit) on-wiki, where it was already raised on-wiki while we discuss. It should be possible to parse and break down the titles into discrete batches, but that's only going to make the above issue much worse.
I really don't like lying to folks by showing these links, but I think that's worse. If it's noticeable enough to be an issue on their loading, users will certainly notice them leaving. Best-case scenario it's confusing, worst-case is zOMG I iz haxed. Anyway, I think the best thing to do is basically what I did on-wiki: revert the contributions changes, leaving in the check on diffs. I think we can be smarter with |
Disappearing of links will be noticeable, but I think that's ok. We can split the pagelist to batches of 50 (the limit is 50 for sysops too from what I read) and do the api calls after the links have been loaded. I really think this is the way to go. I don't think this "lying" (for a few seconds, after we do the above) is an issue. Twinkle has been an extensively popular tool for reverting vandalism for 12 years, but how many times has anyone complained about this? Never, right? |
…ally revert wikimedia-gadgets#632) `intestactions` is limited to 50 unique pages for most users, so this would short-circuit otherwise. See discussion in wikimedia-gadgets#632
|
@siddharthvp I just merged #658, which undoes the relevant bit as you requested (also covers the case of I'd be curious to see how it looks broken into batches, but I don't think it's urgent. I still think placing then removing links would be worse than the current/original situation. The interface knows which pages are editable, or at least which pages are rollbackable, of which editable is one facet. I don't think it's explicitly exposed, though, but it'd be nice if it were. |
…emove auto function As far as I can tell, all the desired information is already available, thanks to some classes, so there should be no need to unnecessarily load a page. The revert query itself should be able to handle the case where someone else has edited the page, so we don't really need the `auto` behavior now that we have the information ready. The other loss is handling of pages that fail `wgIsProbablyEditable`, but this is manageable by making use of `intestactions=edit` (see also wikimedia-gadgets#632 and wikimedia-gadgets#658). By not relying on auto-processing `twinklerevert` urls, we can prevent malicious providing of a link with `&twinklerevert=vand` to entice someone to revert something. Low-profile, and I've never heard of it, but possible. Given the above, `autorevert` has been removed in favor of a `skipTalk` item modified on contribs and recent changes to more appropriately represent what `openTalkPageOnAutoRevert` does. That config is unfortunately named, but isn't visible so shouldn't be an issue beyond maintenance.
…emove auto function As far as I can tell, all the desired information is already available, thanks to some classes, so there should be no need to unnecessarily load a page. The revert query itself should be able to handle the case where someone else has edited the page, so we don't really need the `auto` behavior now that we have the information ready. The other loss is handling of pages that fail `wgIsProbablyEditable`, but this is manageable by making use of `intestactions=edit` (see also wikimedia-gadgets#632 and wikimedia-gadgets#658). By not relying on auto-processing `twinklerevert` urls, we can prevent malicious providing of a link with `&twinklerevert=vand` to entice someone to revert something. Low-profile, and I've never heard of it, but possible. Given the above, `autorevert` has been removed in favor of a `skipTalk` item modified on contribs and recent changes to more appropriately represent what `openTalkPageOnAutoRevert` does. That config is unfortunately named, but isn't visible so shouldn't be an issue beyond maintenance.
…emove auto function As far as I can tell, all the desired information is already available, thanks to some classes, so there should be no need to unnecessarily load a page. The revert query itself should be able to handle the case where someone else has edited the page, so we don't really need the `auto` behavior now that we have the information ready. The other loss is handling of pages that fail `wgIsProbablyEditable`, but this is manageable by making use of `intestactions=edit` (see also wikimedia-gadgets#632 and wikimedia-gadgets#658). By not relying on auto-processing `twinklerevert` urls, we can prevent malicious providing of a link with `&twinklerevert=vand` to entice someone to revert something. Low-profile, and I've never heard of it, but possible. Given the above, `autorevert` has been removed in favor of a `skipTalk` item modified on contribs and recent changes to more appropriately represent what `openTalkPageOnAutoRevert` does. That config is unfortunately named, but isn't visible so shouldn't be an issue beyond maintenance.
…emove auto function As far as I can tell, all the desired information is already available, thanks to some classes, so there should be no need to unnecessarily load a page. The revert query itself should be able to handle the case where someone else has edited the page, so we don't really need the `auto` behavior now that we have the information ready. The other loss is handling of pages that fail `wgIsProbablyEditable`, but this is manageable by making use of `intestactions=edit` (see also wikimedia-gadgets#632 and wikimedia-gadgets#658). By not relying on auto-processing `twinklerevert` urls, we can prevent malicious providing of a link with `&twinklerevert=vand` to entice someone to revert something. Low-profile, and I've never heard of it, but possible. Given the above, `autorevert` has been removed in favor of a `skipTalk` item modified on contribs and recent changes to more appropriately represent what `openTalkPageOnAutoRevert` does. That config is unfortunately named, but isn't visible so shouldn't be an issue beyond maintenance.
Closes #605. Basically, we need a way to check for ability to edit the covers both explicit (protection) and implicit (namespace and content model) restrictions.
Since we're on the page in question,
diffandoldid(#562) can be handled together with mostly-accurate results by simply checkingwgIsProbablyEditable. It won't handle cascading protection or TitleBlacklist restrictions, but neither does MediaWiki when choosing to display either "edit this page" or "view source"; being more accurate than the software would require an API check a la contributions (see following section as well as https://gerrit.wikimedia.org/r/c/mediawiki/core/+/65009).contributionsis the morecomplexinteresting implementation, since we've got to check each and every page. Querying foreditinintestactionsis excellent, because it covers all bases (i.e., it includes the "expensive" checks thatwgIsProbablyEditableskips, henceProbably). I also restructured/split the contributions routine to avoid doingasyncand to look more Twinkley.intestactionswas added in MW 1.25 (https://www.mediawiki.org/wiki/MediaWiki_1.25) circa 2014-2015, and should probably be used more frequently by Twinkle, particularly inMorebits, as it's a more correct determiner of whether the user can edit (or move or protect or...) the page in question.The first commit (remove redundant checks; better way of doing #630 aka e0f3d9c) is worth doing regardless.