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 Frostly."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

SubjectRepoBranchLines +/-
mediawiki/extensions/FlaggedRevsmaster+48 -75
mediawiki/extensions/FlaggedRevsmaster+51 -73
mediawiki/extensions/FlaggedRevsmaster+1 -31
mediawiki/extensions/FlaggedRevsmaster+5 -0
mediawiki/extensions/FlaggedRevsmaster+99 -190
mediawiki/extensions/FlaggedRevsmaster+77 -60
mediawiki/extensions/FlaggedRevsmaster+155 -288
operations/mediawiki-configmaster+11 -33
mediawiki/extensions/FlaggedRevsmaster+18 -20
mediawiki/extensions/FlaggedRevsmaster+25 -48
mediawiki/extensions/FlaggedRevsmaster+0 -86
mediawiki/extensions/FlaggedRevsmaster+7 -9
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+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+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 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

Change 678136 abandoned by Thiemo Kreuz (WMDE):

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

Reason:

I checked and it looks like all this was already done as part of other patches. Sorry, I somehow missed this one here.

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

Change 896102 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/extensions/FlaggedRevs@master] FlaggableWikiPage: inline $callback

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

Change 896102 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] FlaggableWikiPage: inline $callback

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

Change 790707 merged by jenkins-bot:

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

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

Mentioned in SAL (#wikimedia-operations) [2023-03-09T14:54:02Z] <zabe@deploy2002> Started scap: Backport for [[gerrit:790707|Drop unused FlaggedRevs threshold level names (T277883)]]

Mentioned in SAL (#wikimedia-operations) [2023-03-09T14:55:52Z] <zabe@deploy2002> awight and zabe: Backport for [[gerrit:790707|Drop unused FlaggedRevs threshold level names (T277883)]] synced to the testservers: mwdebug1001.eqiad.wmnet, mwdebug2001.codfw.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2002.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-03-09T15:04:51Z] <zabe@deploy2002> Finished scap: Backport for [[gerrit:790707|Drop unused FlaggedRevs threshold level names (T277883)]] (duration: 10m 48s)

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

[mediawiki/extensions/FlaggedRevs@master] Replace review CSS with Codex components

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

Change 930927 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Replace review CSS with Codex components

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

Change 936356 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/extensions/FlaggedRevs@master] Add type declarations in frontend/

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

Change 936357 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/extensions/FlaggedRevs@master] RevisionReviewFormUI: Remove some traces of the tiers

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

Change 936356 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Add type declarations in frontend/

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

Change 938013 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/extensions/FlaggedRevs@master] Hide level zero in the revision review form

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

Change 936357 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] RevisionReviewFormUI: Remove some traces of the tiers

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

Change 938013 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Hide level zero in the revision review form

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

Change 944251 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/extensions/FlaggedRevs@master] RevisionReviewForm: store single tag

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

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

[mediawiki/extensions/FlaggedRevs@master] Deprecate FlaggedRevsRevisionReviewFormAfterDoSubmit

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

Change 945917 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Deprecate FlaggedRevsRevisionReviewFormAfterDoSubmit

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

Change 944251 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] RevisionReviewForm: store single tag

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

Change 673240 abandoned by Bartosz Dziewoński:

[mediawiki/extensions/FlaggedRevs@master] Drop FlaggedRevsRevisionReviewFormAfterDoSubmit Hook

Reason:

It looks like there's a good reason for this hook.

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