Page MenuHomePhabricator

Drop all low-use and unused features of FlaggedRevs to make it more maintainable
Open, Needs TriagePublic

Description

Split from parent per @Tgr.

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/FlaggedRevsmaster+5 -721
mediawiki/extensions/FlaggedRevsmaster+1 -1 K
mediawiki/extensions/FlaggedRevsmaster+6 -3 K
mediawiki/extensions/FlaggedRevsmaster+0 -45
mediawiki/extensions/FlaggedRevsmaster+1 -97
mediawiki/extensions/FlaggedRevsmaster+19 -588
mediawiki/extensions/FlaggedRevsmaster+38 -71
mediawiki/extensions/FlaggedRevsmaster+2 -2
mediawiki/extensions/FlaggedRevswmf/1.37.0-wmf.9+0 -44
mediawiki/extensions/FlaggedRevsmaster+0 -44
mediawiki/extensions/FlaggedRevsmaster+7 -66
mediawiki/extensions/FlaggedRevsmaster+27 -51
mediawiki/extensions/FlaggedRevsmaster+0 -27
mediawiki/extensions/BlueSpiceFlaggedRevsConnectormaster+27 -60
mediawiki/extensions/FlaggedRevsmaster+0 -21
mediawiki/extensions/FlaggedRevsmaster+25 -48
mediawiki/extensions/FlaggedRevsmaster+13 -23
mediawiki/extensions/FlaggedRevsmaster+1 -31
operations/mediawiki-configmaster+11 -11
mediawiki/extensions/FlaggedRevsmaster+18 -144
mediawiki/extensions/FlaggedRevsmaster+0 -221
mediawiki/extensions/FlaggedRevsmaster+0 -1 K
mediawiki/extensions/FlaggedRevsmaster+96 -319
translatewikimaster+0 -13
mediawiki/extensions/FlaggedRevsmaster+154 -155
mediawiki/extensions/FlaggedRevsmaster+0 -3
translatewikimaster+0 -22
mediawiki/extensions/FlaggedRevsmaster+0 -1 K
mediawiki/extensions/FlaggedRevsmaster+13 -21
mediawiki/extensions/FlaggedRevsmaster+3 -1 K
translatewikimaster+0 -5
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 674424 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Clean up two barely used frontend features

https://gerrit.wikimedia.org/r/674424

Change 677412 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[operations/mediawiki-config@master] flaggedrevs: Disable quality and pristine tier in all wikis

https://gerrit.wikimedia.org/r/677412

Change 677484 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/BlueSpiceFlaggedRevsConnector@master] Remove a lot of broken/deleted/unused code

https://gerrit.wikimedia.org/r/677484

Change 677412 merged by jenkins-bot:

[operations/mediawiki-config@master] flaggedrevs: Disable quality and pristine tier in all wikis

https://gerrit.wikimedia.org/r/677412

Mentioned in SAL (#wikimedia-operations) [2021-04-07T11:15:10Z] <ladsgroup@deploy1002> Synchronized wmf-config/flaggedrevs.php: [[gerrit:677412|flaggedrevs: Disable quality and pristine tier in all wikis]] (T277883) (duration: 02m 15s)

Change 678129 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Drop FR_INCLUDES_FREEZE

https://gerrit.wikimedia.org/r/678129

Change 678131 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Drop FlaggedRevsXML::getLevelMenu()

https://gerrit.wikimedia.org/r/678131

Change 678136 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Cleanups after removing extra tiers and extra dimensions

https://gerrit.wikimedia.org/r/678136

Change 678129 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop FR_INCLUDES_FREEZE

https://gerrit.wikimedia.org/r/678129

Change 678131 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop FlaggedRevsXML::getLevelMenu()

https://gerrit.wikimedia.org/r/678131

Unwanted regression. CSS-classes for the lines in the history/recentchanges etc lists which will tell if the revision is reviewed or not is gone. An example light blue lines in this screenshot for the edits reviewed by Opa.

Pending changes lines are still highlighted with yellow so the problem is probably related to removing the tiers. However, the ability to easily see which revisions are reviewed or not is a basic usability issue for the reviewers so the functionality (CSS-classes for the rows which will tell the review status) should be implemented in some other way.

Unwanted regression. CSS-classes for the lines in the history/recentchanges etc lists which will tell if the revision is reviewed or not is gone.

Task is T279750: [accepted revision] is white in FlaggedRevs diffs.

Change 677484 merged by Pwirth:

[mediawiki/extensions/BlueSpiceFlaggedRevsConnector@master] Remove a lot of broken/deleted/unused code

https://gerrit.wikimedia.org/r/677484

Change 680757 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Drop {{#pagesusingpendingchanges}}

https://gerrit.wikimedia.org/r/680757

Change 680757 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop {{#pagesusingpendingchanges}}

https://gerrit.wikimedia.org/r/680757

Change 682241 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Simplify data flow since we support only one type of tag

https://gerrit.wikimedia.org/r/682241

Change 682241 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Simplify data flow since we support only one type of tag

https://gerrit.wikimedia.org/r/682241

The pending changes notification, displayed above articles with pending changes, displays the same text twice, see

image.png (211×1 px, 32 KB)

I assume this should be removed too.

The article on screenshot is https://ru.wikipedia.org/wiki/Хемофобия?uselang=en

If we can decide on what UI mode to go with (low-profile or high-profile, simple or non-simple). Cleaning it up would be much easier.

Is it possible to behave / look like normal notifications (ie upper div box in beginning of the page) and then use CSS/Javascript to modify that for getting the low profile functionality for the wikis/user who like to have low profile ui?

Change 691615 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Drop several configs that are not used in production

https://gerrit.wikimedia.org/r/691615

Change 691615 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop several configs that are not used in production

https://gerrit.wikimedia.org/r/691615

Change 699381 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Drop LocalFile::getHistory hook handler

https://gerrit.wikimedia.org/r/699381

Change 699381 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop LocalFile::getHistory hook handler

https://gerrit.wikimedia.org/r/699381

Change 700340 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@wmf/1.37.0-wmf.9] Drop LocalFile::getHistory hook handler

https://gerrit.wikimedia.org/r/700340

Change 700340 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@wmf/1.37.0-wmf.9] Drop LocalFile::getHistory hook handler

https://gerrit.wikimedia.org/r/700340

Change 703040 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] [WIP] Drop support for recursive stable check

https://gerrit.wikimedia.org/r/703040

Change 705159 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/FlaggedRevs@master] i18n: Don't link to removed Special:ReviewedVersions

https://gerrit.wikimedia.org/r/705159

Change 705159 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] i18n: Don't link to removed Special:ReviewedVersions

https://gerrit.wikimedia.org/r/705159

Change 709218 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[mediawiki/extensions/FlaggedRevs@master] Avoid using fr_quality

https://gerrit.wikimedia.org/r/709218

Change 709218 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Avoid using fr_quality

https://gerrit.wikimedia.org/r/709218

FYI. This is REL1_35 (excluding json)

github.com/AlDanial/cloc v 1.82  T=0.17 s (736.5 files/s, 137763.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
PHP                             87           1768           4349          15222
JavaScript                       3             53            186            481
SQL                             26             65            100            317
CSS                              3             68             38            296
XML                              1              0              0             27
Lua                              2              8              2             26
Markdown                         1              0              0              1
-------------------------------------------------------------------------------
SUM:                           123           1962           4675          16370
-------------------------------------------------------------------------------

And this is master:

github.com/AlDanial/cloc v 1.82  T=0.13 s (754.3 files/s, 156947.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
PHP                             76           1590           4751          12824
JavaScript                       3             50            184            464
CSS                              3             51             28            243
SQL                             13             52             66            236
Lua                              2              8              2             26
XML                              1              2              0             21
Markdown                         1              0              0              1
-------------------------------------------------------------------------------
SUM:                            99           1753           5031          13815
-------------------------------------------------------------------------------

That's a 15% drop in LOC which is not a great measure but better than nothing.

Just FYI, it seems that there is still links to Special:ReviewedVersions in UI in production per discussion in fiwiki.

Edit: It seems that it was outdated translation in translatewiki

Change 703040 abandoned by Ladsgroup:

[mediawiki/extensions/FlaggedRevs@master] [WIP] Drop support for recursive stable check

Reason:

The image part is done and we are not going to do the template part.

https://gerrit.wikimedia.org/r/703040

There is an option in stabilization interface, "Restrict reviewing this page only to sysops", see https://ru.wikipedia.org/wiki/Special:Stabilization/Deus_Ex . We considered in ruwiki that this option is useless and every time, when someone turns it on (very rare), he did it accidentally and this option turns off every time when any user draws attention to this.

We request to disable this option for ruwiki (local discussion: https://ru.wikipedia.org/w/index.php?title=Project:Форум/Предложения&oldid=116710844#Убрать_опцию_%22патрулирование_только_администраторами%22 ) and I assume this option is useless in any wiki, not only ruwiki, so it can be removed completely from FlaggedRevs.

This is the main mode of FlaggedRevs used in enwiki, and is quite popular there. In dewiki, it is used occasionally as well.

This is the main mode of FlaggedRevs used in enwiki, and is quite popular there. In dewiki, it is used occasionally as well.

I think protect mode is different from what is suggested here. Protect mode is when an admin can change stability settings of a page, like who can edit it, who needs review, who would be auto-reviewed, etc. but the option outlined here is when only an admin can review edits (similar to full protect). I haven't seen that being used in my wiki at all.

Change 721995 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@master] Drop option of different levels of auto-review in Special:Stabilization

https://gerrit.wikimedia.org/r/721995

Last time it was used there was 5.5 years ago 🤓🤓🤓

Change 723707 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@master] Drop UnreviewedPages API, Special page and related right

https://gerrit.wikimedia.org/r/723707

@Ladsgroup why you think UnreviewedPages is needless special page?

See the change summary: "It is basically duplicating the whole concept of Special:PendingChanges"

I was told that I thought wrong and it does some stuff differently than Special:PendingChanges namely new pages. My home wiki uses flaggedrevs in protect mode and I didn't know this. I still think that special page should be dropped but its functionality should be merged into one

Special:UnreviewedPages is a list of unreviewed pages (pages has no reviewed versions at this time).
Special:PendingChanges is a list of pages, that has old reviewed version, but its last version is unreviewed.
It's totally different lists, unreviewed and outdated reviewed pages.

Could documentation tasks for example editing https://translatewiki.net/wiki/Translating:Flagged_Revs_extension be included in this task or should be in a new ticket?

Not directly related but we possibly should drop some parts of flaggedtemplates table: T296380

Change 745993 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@master] api: Remove watchlist support

https://gerrit.wikimedia.org/r/745993

Change 745993 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] API: Remove deprecated watchlist support

https://gerrit.wikimedia.org/r/745993

I've been looking at UnreviewedPages, which uses two different mechanisms: if the query is expected to be fast (basically it lists page rows not having a corresponding flaggedpages row, using a left join; fast is defined a the count of all pages minus the size of the flaggedpages table being more than 0.25% of the count of all pages), it does the query in realtime; otherwise it uses the querycache mechanism. That means the DB logic is duplicated. I wonder if that's worth maintaining, as opposed to just always using the cache.

Yes, let's use cache instead. Worst case, we can reduce the cache TTL (by running it more often).

Change 723707 abandoned by Ladsgroup:

[mediawiki/extensions/FlaggedRevs@master] Drop UnreviewedPages API, Special page and related right

Reason:

It is used. I misunderstood its functionality. I will make a patch to possibly either merge the functionality into one special page or simplify the logic by DRY.

https://gerrit.wikimedia.org/r/723707

Yes, let's use cache instead. Worst case, we can reduce the cache TTL (by running it more often).

Freshness isn't the only tradeoff there; the list can be filtered by category and if it's cached that means you can get few/no results for a given category just because it did not appear in the first 5000 results (which is what gets cached). I think it's fine - the large flagrev wikis tend to do a good job of reviewing pages promptly, which means they already get the cached version, as their unreviewed:total ratio is low.

Re: merging UnreviewedPages with PendingPages, the fundamental difference there is that pending pages are those with an flaggedpages entry that marks them as flagged, so you can efficiently query them in real time by joining on that. Unreviewed pages OTOH are those with no flaggedpages entry, and given that the overwhelming majority of pages is expected to be reviewed, that's not going to be efficient. And pending pages are kind of time-sensitive (penbding edits are hidden from the reader which makes the authors of those edits unhappy) while unreviewed pages are not (they are shown with an inobtrusive warning but are not hidden); so PendingPages should probably be a realtime query while UnreviewedPages needs to be cached (short of some significant changes in DB handling, like adding flaggedpages entries for unreviewed pages, which actually might be a good idea). So I'm not sure much can be unified about them.

Change 752228 had a related patch set uploaded (by Gerrit Patch Uploader; author: Remagoxer):

[mediawiki/extensions/FlaggedRevs@master] Drop Special:QualityOversight

https://gerrit.wikimedia.org/r/752228

I'm fairly certain FRUserActivity is dead code (or at least barely used). There is no value in objectcache table:

ladsgroup@mwmaint1002:~$ expanddblist flaggedrevs | xargs -I {} sql {} -- -e "select * from objectcache where keyname like '{}:flagged%' limit 100;"
ladsgroup@mwmaint1002:~$

Even though the ttl is several minutes but I repeated the query several times in different times and yet nothing.

I'm fairly certain FRUserActivity is dead code (or at least barely used). There is no value in objectcache table:

Some of it is not related to that cache, like FRUserActivity::numUsersWatchingPage(). The cache is used to show pages as "under review". This is a manual feature:

flagrev-inprogress.png (289×684 px, 55 KB)

so possibly not used much. (It does work, you can test by visiting the same diff with two different review accounts; click "advertise" with one and the other will see a warning instead of the "advertise" link button.)

I assume this feature could be safely removed due to its uselessness for almost anyone.

I assume this feature could be safely removed due to its uselessness for almost anyone.

Yup. +1. If it's so barely used that you can't find anyone using it most of the time in the whole infra. It's not useful to have a lot of code attached to it (and add work to translators, etc.).

The code itself is rather small but it branches to so many places that removing that would reduce a lot of complexity.

Change 756115 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@master] Remove \"under review\" advertisement functionality

https://gerrit.wikimedia.org/r/756115

Was this change https://phabricator.wikimedia.org/T277883#7363935 merged? There is an option about I write here https://phabricator.wikimedia.org/T277883#7363544 , looks like this change was to remove this option, but it still exist in ruwiki: https://ru.wikipedia.org/wiki/Special:Stabilization/Deus_Ex (second options block)