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+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 673492 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Drop ProblemChanges special page

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

Change 673542 merged by jenkins-bot:
[translatewiki@master] [FlaggedRevs] Drop the 'Problem Changes' set, being deleted upstream

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

@Zache btw, If I understood the code correctly, you can still have multiple levels (0, 1, 2, 3, ...) but only one tier. So changes in tiers should not affect your wiki. I double check.

Yup. Tested locally, you can have multiple levels but only one tier.

Not sure if I was able to follow you on how you defined "tiers". In fiwiki there is in user interface

  • stable = alias for any review
  • checked = levels 1, 2
  • quality = level 3

Example for quality revision

Hmm, it seems that seeing the info requires suitable user group. However here is screenshot

@Ladsgroup and others, if you need "editor" user rights in fiwiki to see the flaggedrevs UI please then just say and can add you to the group.

I can rebuild the scenario locally. I made six levels (why not) locally but only one tier:

You'd see the change is level 4 but still called "checked" and not "quality" so that'll change but it is still possible to mark revision as "well-sourced" or other levels.

I understand it's super confusing but that's the reason I want to simplify it.

Ah, you mean that if the user can still select "well-sourced" when the user is reviewing then it is good enough(?) I try to explain again what I tried to say earlier.

In Flaggedrevs the functionality (CSS-classes, filters in the lists, API filters, creating stabilization configs etc) are used by what you call "tiers" (checked, quality, pristine) and if tiers are deleted from the code then you are effectively leaving dummy level value selection to UI without any functionality related to the selected value.

The biggest part of the confusion related to this is is that the FlaggedRevs configuration used to be matrixes ( dimensions * levels ) which would tell in which tier the page is. However, if we just define that there is just one dimension write example documentation so that there is a 1:1 relation between tiers and level then it would be a lot less confusing and it would be working consistently in the UI, API etc.

Ah, you mean that if the user can still select "well-sourced" when the user is reviewing then it is good enough(?) I try to explain again what I tried to say earlier.

In Flaggedrevs the functionality (CSS-classes, filters in the lists, API filters, creating stabilization configs etc) are used by what you call "tiers" (checked, quality, pristine) and if tiers are deleted from the code then you are effectively leaving dummy level value selection to UI without any functionality related to the selected value.

They are not technically useless, the data can be queried and used later like to train machine learning or other parts but I agree it wouldn't be that useful.

The biggest part of the confusion related to this is is that the FlaggedRevs configuration used to be matrixes ( dimensions * levels ) which would tell in which tier the page is. However, if we just define that there is just one dimension write example documentation so that there is a 1:1 relation between tiers and level then it would be a lot less confusing and it would be working consistently in the UI, API etc.

I understand. But the choice here is not between "having tiers" and "not having tiers". It's between "not having tiers" and "undeploying flaggedrevs". The code under the hood is a big mess. My suggestion is to remove the tiers (and keep levels for now) and if needed:

  • Either implemented later in a better way inside flagged revs
  • or have a new small self-contained extension to handle that. I know there was some efforts in WMF to write something to allow users mark a revision/page in a structured way but I think this is out of scope of this extension. the extension job is about "pending changes" and should only do that (and do it well)

Change 673240 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Drop FlaggedRevsRevisionReviewFormAfterDoSubmit Hook

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

Why this extension has 66 hook handlers :(((

Change 673666 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Drop Special:ReviewedPages

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

Change 673668 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[translatewiki@master] [FlaggedRevs] Drop ReviewedPages

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

I understand. But the choice here is not between "having tiers" and "not having tiers". It's between "not having tiers" and "undeploying flaggedrevs". The code under the hood is a big mess. My suggestion is to remove the tiers (and keep levels for now) and if needed:

  • Either implemented later in a better way inside flagged revs
  • or have a new small self-contained extension to handle that. I know there was some efforts in WMF to write something to allow users mark a revision/page in a structured way but I think this is out of scope of this extension. the extension job is about "pending changes" and should only do that (and do it well)

OK, I suppose that this good enough. It would be anyway a bad idea to keep symbolic tiers in code that are defined using dimension+level combinations if we are trying to get rid of dimensions. The better way would be to store level values to DB as int (like fr_quality is used now) and use that. However, I am a little bit vary that this would actually happen.

Anyway, just to be said aloud. You have my support for removing the tiers. Thank you for cleaning up the code.

Change 673670 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Merge two methods to one

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

PHPCS should be checked.

Generic.Files.LineLength no fails (exclude can be removed), MediaWiki.Usage.DbrQueryUsage.DbrQueryFound fails only for this.

diff --git a/.phpcs.xml b/.phpcs.xml
index 72b7fbce..d10e6c94 100644
--- a/.phpcs.xml
+++ b/.phpcs.xml
@@ -3,13 +3,9 @@
        <rule ref="./vendor/mediawiki/mediawiki-codesniffer/MediaWiki">
                <exclude name="MediaWiki.Commenting.FunctionComment.MissingDocumentationPrivate" />
                <exclude name="MediaWiki.NamingConventions.LowerCamelFunctionsName.FunctionName" />
-               <exclude name="MediaWiki.Usage.DbrQueryUsage.DbrQueryFound" />
                <exclude name="MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment" />
        </rule>
 
-       <rule ref="Generic.Files.LineLength">
-               <exclude-pattern>frontend/language/</exclude-pattern>
-       </rule>
        <rule ref="MediaWiki.Files.ClassMatchesFilename.NotMatch">
                <exclude-pattern>maintenance/*.php</exclude-pattern>
        </rule>

if tiers are deleted from the code then you are effectively leaving dummy level value selection to UI without any functionality related to the selected value.

Tiers other than checked never really had any functionality in the first place, AFAIK. In theory you could configure a page or a whole wiki to use the quality or pristine version as the stable version (the one shown to anons), but I don't think anyone does that.

In Finnish Wikipedia, they are used so that there is some set of articles (say featured articles) which edits are checked as normally for vandalism and then there is also the slower round where users are doing time to time more deeply checking what is changed after last quality revision.

The second use has been that we have needed article versions that are checked as vandalism-free because they are used in some visible place. One idea was to use the content of Finnish Wikipedia in the Finnish Game Museum exhibition so that text was updating when edits to Wikipedia articles were approved to a certain level.

However, We haven't used FlaggedRevs for showing stable versions to anon users inside Finnish Wikipedia and its main use is coordinating the checking of the edits.

Change 673670 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Drop useSimpleConfig(), it's just an alias for useOnlyIfProtected()

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

Change 673666 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Drop Special:ReviewedPages

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

Change 673668 merged by jenkins-bot:
[translatewiki@master] [FlaggedRevs] Drop ReviewedPages

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

I see that rEFLR0fe4ed9a6842: Drop FlaggedRevsWhitelist option and just disable it on main page removes the review process for the main page. That does not work for all wikis equally. Russian Wikipedia, for example, has the main page stabilised, and FlaggedRevs is used there to prevent unreviewed edits from the templates showing up on the main page. With the reviewing disabled on it, they would all show immediately due to how FlaggedRevs propagates changes. Protecting all the templates to sysops is not exactly an option, since a lot of templates are updated by regular editors.

Thanks for the note, I missed it as the icon was hidden in the main page. This is basically another case of Hyrum's law. FlaggedRevs is not designed for this and I ask you to protect the wiki like basically all other wikis (with daily changes of the templates + cascading protection) and ruwiki is big enough for that. If it's really disruptive, and there's a consensus, I will add it back in a simpler form (a boolean, which reduces the dimension in the space of possibilities)

What's the point of exempting the main page? Hiding unreviewed versions can be disabled for the main page, like for any other page, via Special:Stabilization. That won't hide the review UI but its presence does not seem problematic.

So another point to clean up is rights, we have these rights:

  • "review"
    • OK
  • "validate",
    • God like, full review right on everything. Can be dropped once we removed tiers and dimensions.
  • "autoreview"
    • OK
  • "autoreviewrestore"
    • Why?
  • "unreviewedpages"
    • Really? Why?
  • "movestable"
    • Sounds useful but broken and can be simply moved to autoreview
  • "stablesettings"
    • OK

So another point to clean up is rights, we have these rights:

  • "validate",
    • God like, full review right on everything. Can be dropped once we removed tiers and dimensions.

Not related to tiers or dimensions but levels. $wgFlaggedRevsTagsRestrictions defines which is max level which can be reviewed or autoreviewed with review right.

Ie. if you have 2 levels (say "vandal free" and "validated") and $wgFlaggedRevsTagsRestrictions = [ 'review' => 1, 'autoreview' => 1 ] then users with autoreview and reviewer right would review it always to 1 and users with validate can review article later back to level 2.

  • "autoreviewrestore"
    • Why?

I guess because by default if users with autoreview right will edit article with unchecked unreviewed edits his edits are unreviewed. If this setting is true then if the edit is rollbacked to reviewed edit then latest version will be automatically reviewed. ( this should be checked from the code though)

  • "unreviewedpages"
    • Really? Why?

Afaik security by obscrusity. Do not show list of pages which arent followed using flagged revs to potential vandals.

I guess because by default if users with autoreview right will edit article with unchecked unreviewed edits his edits are unreviewed. If this setting is true then if the edit is rollbacked to reviewed edit then latest version will be automatically reviewed. ( this should be checked from the code though)

Confirmed, users with this permission autoreview when they rollback to a stable version. In WMF config it's given to rollbackers.

Seems pointless to me - in what situation would we not want such a rollback to be autoreviewed? For manual reverts, maybe one might want to check whether the edit summary contains something inappropriate, or that it's not unwarranted supression of a good edit from a non-autoreviewed editor - but rollback is only handed out to trusted users in the first place.

Afaik security by obscrusity. Do not show list of pages which arent followed using flagged revs to potential vandals.

IIRC Special:UnreviewedPages shows the number of watching users (and by extension, which pages are not on anyone's watchlist) which some wikis worry could be abused for vandalism / disinformation purposes.

Thanks for the note, I missed it as the icon was hidden in the main page. This is basically another case of Hyrum's law. FlaggedRevs is not designed for this and I ask you to protect the wiki like basically all other wikis (with daily changes of the templates + cascading protection) and ruwiki is big enough for that. If it's really disruptive, and there's a consensus, I will add it back in a simpler form (a boolean, which reduces the dimension in the space of possibilities)

Looking into https://noc.wikimedia.org/conf/highlight.php?file=flaggedrevs.php, only 3 wikis (arwiki, hewikisource, ruwikinews) have $wgFlaggedRevsWhitelist set. Out of those 3 wikis, ruwikinews configured it wrong for years, since their main page is located not at Main_Page (which is indeed unreviewable) but at Заглавная_страница. Since FlaggedRevs is installed at 45 wikis, I think removing the review process from the main pages that have it right now (out of Wikipedias: sqwiki, bswiki, bewiki, cewiki, kawiki, huwiki, idwiki, iawiki, mkwiki, ruwiki, trwiki, ukwiki, vecwiki) would be far worse than telling to arwiki and hewikisource to take whatever actions they deem necessary after this. For example, it is really easy to just move the main page into a different namespace if you don’t want it reviewed, but there’s no way to drop the config flag from wgIsMainPage from it.

Change 673834 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Drop excluding the main page from Pending changes

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

  • "movestable"
    • Sounds useful but broken and can be simply moved to autoreview

Sounds like a confusing solution to me.

In particular, would be a breaking change for most wikis including dewiki, where autoconfirmed users are allowed to move pages, but lack autoreview permissions.

Change 673834 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Drop excluding the main page from Pending changes

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

Announcement is sent to all wikis that have this extension enabled and the branch is cut. I'll do another round of clean (including quality tier)

Change 674422 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Drop Special:ReviewedVersions

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

Change 674423 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Drop quality tier

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

Change 674424 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Clean up two barely used frontend features

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

I'm cleaning up two frontend bits:

The first one gives a background image on a non-low profile wiki (two
wikis, including Hungarian Wikipedia) and when a page has a pending edit
and you're logged out and seeing the old version instead and you hover
on the eye icon. You'd see this background which actually makes it
harder to read (violating WCAG), without it it just looks fine.

The second one is even worse. The wiki need to be non-simple UI (only
three wikis), and has to have more than two levels (only English
Wikibooks), you need to have reviewer right, check out an old stable revision
(with oldid=) and then it's hidden under "display: none" on the message
box, you can inspect element it or with javascript show that +/- which
by clicking shows a bar of progress.


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

[translatewiki@master] [FlaggedRevs] Drop the 'Reviewed Versions' set, being deleted upstream

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

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

[mediawiki/extensions/FlaggedRevs@master] Drop ApiQueryReviewedpages

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

Change 676735 merged by jenkins-bot:

[translatewiki@master] [FlaggedRevs] Drop the 'Reviewed Versions' set, being deleted upstream

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

Change 674423 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop quality tier

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

Change 674422 merged by jenkins-bot:

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

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

Change 676737 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Drop ApiQueryReviewedpages

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

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