Page MenuHomePhabricator

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

Assigned To
Authored By
Jdforrester-WMF
Mar 19 2021, 4:41 PM
Referenced Files
F34921363: flagrev-inprogress.png
Jan 18 2022, 12:28 AM
F34448546: image.png
May 10 2021, 3:46 AM
F34183346: image.png
Mar 23 2021, 8:12 PM
F34183348: image.png
Mar 23 2021, 8:12 PM
F34183343: image.png
Mar 23 2021, 8:12 PM
F34174234: image.png
Mar 20 2021, 4:00 AM
Tokens
"Mountain of Wealth" token, awarded by EpicPupper."Like" token, awarded by Asartea."Like" token, awarded by Pppery."Like" token, awarded by Mainframe98."Yellow Medal" token, awarded by Kizule."Love" token, awarded by Aklapper."Yellow Medal" token, awarded by Tgr.

Description

Split from parent per @Tgr.

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/FlaggedRevsmaster+0 -86
mediawiki/extensions/FlaggedRevsmaster+7 -9
operations/mediawiki-configmaster+11 -33
mediawiki/extensions/FlaggedRevsmaster+41 -67
mediawiki/extensions/FlaggedRevsmaster+11 -34
mediawiki/extensions/FlaggedRevsmaster+2 -36
mediawiki/extensions/FlaggedRevsmaster+1 -1 K
mediawiki/extensions/FlaggedRevsmaster+21 -30
mediawiki/extensions/FlaggedRevsmaster+1 -31
mediawiki/extensions/FlaggedRevsmaster+37 -70
mediawiki/extensions/FlaggedRevsmaster+0 -85
mediawiki/extensions/FlaggedRevsmaster+68 -142
mediawiki/extensions/FlaggedRevsmaster+5 -721
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
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

StatusSubtypeAssignedTask
OpenNone
StalledNone
StalledNone
StalledNone
StalledNone
OpenNone
OpenLadsgroup
ResolvedLadsgroup
DeclinedMarostegui
ResolvedLadsgroup
ResolvedLadsgroup
ResolvedMarostegui
ResolvedLadsgroup
ResolvedBUG REPORTNone
ResolvedLadsgroup
ResolvedLadsgroup
ResolvedMarostegui
ResolvedMarostegui
ResolvedMarostegui
ResolvedMarostegui
ResolvedMarostegui
DeclinedTrizek-WMF
ResolvedLadsgroup
ResolvedMarostegui
ResolvedZabe
ResolvedLadsgroup
ResolvedLadsgroup
OpenLadsgroup

Event Timeline

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

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)

Change 756115 merged by jenkins-bot:

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

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

@MBH no that referenced commit has not been merged.

And... why, and when it will be merged?

It breaks protection form, I didn't have time to fully look why it's breaking it but I can try later.

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

[mediawiki/extensions/FlaggedRevs@master] Drop fr_img_* columns

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

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

[mediawiki/extensions/FlaggedRevs@master] Drop orphan PG schema change sql files

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

Change 757949 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop fr_img_* columns

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

Change 757953 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop orphan PG schema change sql files

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

Note that this is not stalled. I have been cleaning up its useless data in production but there are just so much of them. Already over 1TB of clean up now.

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

[mediawiki/extensions/FlaggedRevs@master] Simplify the code a bit

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

Change 752228 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop Special:AdvancedReviewLog (a.k.a. Special:QualityOversight)

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

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

[mediawiki/extensions/FlaggedRevs@master] Inline a lot of single-use code in FlaggedRevs util class

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

Change 773946 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Simplify the code a bit

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

Change 774898 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Inline a lot of single-use code in FlaggedRevs util class

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

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

[mediawiki/extensions/FlaggedRevs@master] Drop leftovers from the removed "quality" tier

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

Change 790701 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/FlaggedRevs@master] Clean up unused quality tier thresholds

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

Change 790707 had a related patch set uploaded (by Awight; author: Awight):

[operations/mediawiki-config@master] Drop unused FlaggedRevs threshold level names

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

Change 790756 had a related patch set uploaded (by Ladsgroup; author: Bartosz Dziewoński):

[mediawiki/extensions/FlaggedRevs@master] FlaggablePageView: Use DifferenceEngine helper methods

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

Change 790756 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] FlaggablePageView: Use DifferenceEngine helper methods

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

Change 777366 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop leftovers from the removed "quality" tier

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

Change 817932 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/FlaggedRevs@master] Rearrange and cull review-related JS

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

Change 817932 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Rearrange and cull review-related JS

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

Change 790701 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Clean up unused quality tier thresholds

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

Change 860967 had a related patch set uploaded (by Ladsgroup; author: Bartosz Dziewoński):

[mediawiki/extensions/FlaggedRevs@master] Remove unused maintenance script clearCachedText.php

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

Change 860967 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Remove unused maintenance script clearCachedText.php

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