Page MenuHomePhabricator

Code stewardship review: FlaggedRevs
Open, HighPublic

Description

A succinct problem statement to give context for why the review was initiated.
This extension has been deployed to prod before 2011 (see below) and after a while it became virtually without any maintainer. Technical debt in this code is unimaginable and in matter of UX/UI it's non-standard and outdated but any (of my) attempts to modernize it failed due to out-of-date and non-standard PHP code.

Current developers/maintainers:
No one in particular

Number, severity, and age of known and confirmed security issues:
T234736

Was it a cause of production outages or incidents? List them.

Found this: https://wikitech.wikimedia.org/wiki/Incident_documentation/20170111-multiversion but it might be not that related

Does it have sufficient hardware resources for now and the near future (to take into account expected usage growth)?

Yes, and given the nature of the extension, it's very unlikely it will need more hardware any time soon.

Is it a frequent cause of monitoring alerts that need action, and are they addressed timely and appropriately?
Yes.

The extension is a frequent cause of problems like PHP errors (some fatal), such as T174803.

See more at https://phabricator.wikimedia.org/project/board/296/query/all/.

When it was first deployed to Wikimedia production

Usage statistics based on audience(s) served
More than 40 Wikimedia projects.

Changes committed in last 1, 3, 6, and 12 months
https://gerrit.wikimedia.org/g/mediawiki/extensions/FlaggedRevs/%2Blog/master

Reliance on outdated platforms (e.g. operating systems)
?

Number of developers who committed code in the last 1, 3, 6, and 12 months
?

Number and age of open patches
?

Number and age of open bugs
?

Number of known dependencies?
Unknown.

The configuration page mentions Echo, Scribunto and some other WMF extensions

Is there a replacement/alternative for the feature? Is there a plan for a replacement?
Content approval extensions lists similar extensions, but there is no replacement or alternative mapped or planned. One option would be to undeploy this from production. However, doing so would remove the ability for communities to protect pages in a way that still allows contributions. (In other words, it removes the ability to perform pre-publish edit review.)

Related Objects

StatusSubtypeAssignedTask
OpenNone
DeclinedNone
DeclinedNone
DeclinedNone
DeclinedNone
OpenNone
ResolvedLadsgroup
ResolvedLadsgroup
DeclinedMarostegui
ResolvedLadsgroup
ResolvedLadsgroup
ResolvedMarostegui
ResolvedLadsgroup
ResolvedBUG REPORTNone
ResolvedLadsgroup
ResolvedLadsgroup
ResolvedMarostegui
ResolvedMarostegui
ResolvedMarostegui
ResolvedMarostegui
ResolvedMarostegui
DeclinedTrizek-WMF
ResolvedLadsgroup
ResolvedMarostegui
ResolvedZabe
ResolvedLadsgroup
ResolvedLadsgroup
OpenNone

Event Timeline

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

Something simple like this for Tech News?

Flagged revisions will no longer have extra dimensions like "tone" or "depth". It will also only have one tier. This is because very few wikis use them but they make the tool very difficult to maintain.

Something simple like this for Tech News?

Flagged revisions will no longer have extra dimensions like "tone" or "depth". It will also only have one tier. This is because very few wikis use them but they make the tool very difficult to maintain.

Sound great to me. Thanks.

Flagged revisions will no longer have extra dimensions like "tone" or "depth". It will also only have one tier. This is because very few wikis use them but they make the tool very difficult to maintain.

just scrolling on by but the last sentence reads a bit clunky to me. What about:

Flagged revisions will no longer have extra dimensions like "tone" or "depth" and will only have one tier. This was changed because very few wikis used these features and they make the tool difficult to maintain.

I would keep the wording same as in documentation. Ie. "Flagged revisions will no longer have multiple tags like "accuracy", "tone" or "depth" and will only have single tag"....

How about the levels inside dimensions?

Thank you @Ladsgroup for chipping away at this pile of cruft!

Some wikis use the "quality" tiers to mark the revision of featured articles in which they got featured. The whole concept of quality/pristine comes from WP 1.0 AIUI. I agree it can be dropped, tiers other than "checked" don't result in any actual change in how the article is presented and as such are quite pointless. If we want to highlight community-peer-reviewed versions of an article, that's probably better done as a separate extension. (Also the whole level vs. tier business is super confusing.)

  • Functional complexities: It introduces 18 new special pages, that's crazy.

11, actually.

QualityOversight becomes obselete if the quality tier is removed, Special:ProblemChanges is probably not terribly useful (change tags are used rather differently from what that page assumes), AdvancedReviewLog neither (and these days I think the functionality could just be added to Special:Log). I don't think ReviewedPages has a real purpose (it lists pages which already have been reviewed, not sure what the use case is), ReviewedVersions is basically just a weird filtered version of page history which also seems unnecessary. RevisionReview sounds important but I'm not really sure where it gets used as review happens via the normal page / diff view.

The rest of the special pages are quite useful IMO. (Stabilization/ConfiguredPages/StablePages are for setting / listing pending protection, although I don't really understand the difference between the last two; UnreviewedPages and PendingChanges are two different kinds of work backlog for reviewers, ValidationStatistics shows whether changes get through the system in a reasonable amount of time + who the most prolific reviewers are.)

Thank you @Ladsgroup for chipping away at this pile of cruft!

Some wikis use the "quality" tiers to mark the revision of featured articles in which they got featured. The whole concept of quality/pristine comes from WP 1.0 AIUI. I agree it can be dropped, tiers other than "checked" don't result in any actual change in how the article is presented and as such are quite pointless. If we want to highlight community-peer-reviewed versions of an article, that's probably better done as a separate extension. (Also the whole level vs. tier business is super confusing.)

Ack. I just sent an announcement on this. I will start disabling multiple levels and tiers next week. Then I will start dropping code and simplifying its logic. One nice thing was that by dropping other dimensions, we reduced size of translations needed.

  • Functional complexities: It introduces 18 new special pages, that's crazy.

11, actually.

Yes, I counted classes and some of these special pages have a Pager class attached. FTR, Wikibase has 30 classes too, so its special pages is also lower.

QualityOversight becomes obselete if the quality tier is removed, Special:ProblemChanges is probably not terribly useful (change tags are used rather differently from what that page assumes), AdvancedReviewLog neither (and these days I think the functionality could just be added to Special:Log). I don't think ReviewedPages has a real purpose (it lists pages which already have been reviewed, not sure what the use case is), ReviewedVersions is basically just a weird filtered version of page history which also seems unnecessary. RevisionReview sounds important but I'm not really sure where it gets used as review happens via the normal page / diff view.

The rest of the special pages are quite useful IMO. (Stabilization/ConfiguredPages/StablePages are for setting / listing pending protection, although I don't really understand the difference between the last two; UnreviewedPages and PendingChanges are two different kinds of work backlog for reviewers, ValidationStatistics shows whether changes get through the system in a reasonable amount of time + who the most prolific reviewers are.)

Thank you so much for checking, I will start dropping these special pages soon.

Two other aspects to work on:

  • Frontend. It has two options wgSimpleFlaggedRevsUI that's true everywhere except enwikibooks, fiwiki, and ptwikibooks. Let's just make it true everywhere and drop the "non-simple" ui. wgFlaggedRevsLowProfile is also set to true everywhere except huwiki and ptwikisource.
  • User permission and auto-promote rules, we should introduce a hook for auto-promote in core and use that instead of directly hooking into $wgAutoPromote variable. I haven't checked the user permissions in depth but any suggestion for simplification is more than welcome.
  • User permission and auto-promote rules, we should introduce a hook for auto-promote in core and use that instead of directly hooking into $wgAutoPromote variable. I haven't checked the user permissions in depth but any suggestion for simplification is more than welcome.

The auto promotion or the way the conditions were understood in dewiki is also broken, it apparenty counts something else completely, see e.g. this conversation.

Also the data in the autopromote DB tables does not seem to be consistent with reality, see e.g. this query, which shows that the user has only made 55 edits in dewp (deleted edits are still counted in user_editcount) but according to the flaggedRevs DB they have 705 content edits alone...

BTW: Thanks so much for your work!

QualityOversight becomes obselete if the quality tier is removed, Special:ProblemChanges is probably not terribly useful (change tags are used rather differently from what that page assumes), AdvancedReviewLog neither (and these days I think the functionality could just be added to Special:Log). I don't think ReviewedPages has a real purpose (it lists pages which already have been reviewed, not sure what the use case is), ReviewedVersions is basically just a weird filtered version of page history which also seems unnecessary. RevisionReview sounds important but I'm not really sure where it gets used as review happens via the normal page / diff view.

The rest of the special pages are quite useful IMO. (Stabilization/ConfiguredPages/StablePages are for setting / listing pending protection, although I don't really understand the difference between the last two; UnreviewedPages and PendingChanges are two different kinds of work backlog for reviewers, ValidationStatistics shows whether changes get through the system in a reasonable amount of time + who the most prolific reviewers are.)

Thank you so much for checking, I will start dropping these special pages soon.

AdvancedReviewLog definitely is useful and I believe it should stay

  • Frontend. It has two options wgSimpleFlaggedRevsUI that's true everywhere except enwikibooks, fiwiki, and ptwikibooks. Let's just make it true everywhere and drop the "non-simple" ui. wgFlaggedRevsLowProfile is also set to true everywhere except huwiki and ptwikisource.

I’m not sure if this is in scope, but I suggest also looking more into what default FlaggedRevs configuration displays to readers by default:

image.png (234×1 px, 46 KB)

(logged-out, no review rights)

image.png (243×1 px, 60 KB)

(logged in, have review rights)

This is basically the same text displayed to user twice (and links repeated thrice), ironically enabled with the setting ‘Use small icons and minimal text to show review status of pages’ (not sure if that is the ‘simple’ UI from the setting you are mentioning). The other option for that preference (under Recent changes) leaves just the blue box from the images.

I don’t think anyone would be harmed by at least removing the duplication from this option, even if the setting will stay.

RevisionReview sounds important but I'm not really sure where it gets used as review happens via the normal page / diff view.

It powers all the review forms, provides non-JS interface for FlaggedRevs and is used when people press ‘Reject changes’. So, really better not to delete it.

(Upon looking at the diff interface though, I’ve come to realise that I’ve never used the feature ‘advertise to other users that you are checking the edits’. Maybe others have though.)

I have clicked the ‘advertise to other users that you are checking the edits’ at some point but I can't even remember what it does

If you set this checkbox, users who browse "Pages for reviewing" special page, will see "Some user reviewing this page" notification on the string with this pagename.

Ping @Mglaser - it seems like BlueSpice contains some amount of FlagRev customizations so this might be interesting for your team.

(BTW do we want to move to a dedicated task?)

  • Frontend. It has two options wgSimpleFlaggedRevsUI that's true everywhere except enwikibooks, fiwiki, and ptwikibooks. Let's just make it true everywhere and drop the "non-simple" ui.

FWIW that's also a user preference (flaggedrevssimpleui) so more people could in theory use it.

The description says "This will only distinguish "checked", "quality", and unreviewed" which would suggest that after dimensions and extra tiers are removed, there is no point to the non-simple UI. Looking at the code though, it seems like some of the difference is just between having short and cryptic messages vs. longer and understandable messages, so I'm not sure dropping non-simple is a good idea. (Short and cryptic is IMO one of the main UX problems with the extension - its main goal is to reassure the reader that the content can be trusted, but what should the reader surmise from seeing a little eye icon above the text? Which, when hovered, says things like "unaccepted"?) I agree we shouldn't try to maintain two separate versions, I'm just not sure the simple version is always better.

wgFlaggedRevsLowProfile is also set to true everywhere except huwiki and ptwikisource.

The same goes for this as well. Maybe we could make a UI comparison and show it to a designer before removing stuff?

RevisionReview sounds important but I'm not really sure where it gets used as review happens via the normal page / diff view.

It powers all the review forms, provides non-JS interface for FlaggedRevs and is used when people press ‘Reject changes’. So, really better not to delete it.

Oh, it's the submission target for the forms in the diff and article view, that makes sense.

(Upon looking at the diff interface though, I’ve come to realise that I’ve never used the feature ‘advertise to other users that you are checking the edits’. Maybe others have though.)

It kind of "locks" the page for a few minutes (doesn't prevent others from reviewing but they will see a small message saying "this is already being reviewed by another editor"). Maybe useful on large wikis where a lot of patrollers are working on recent changes in parallel?

"SimpleUI" will currently break the fiwikis page layout as it will collide with some templates. "Non-simple UI" aka basic text box works well when the number of the pages where it is visible is low (like it is in fiwiki) and it will actually notify users that there is new changes to users to check.

"SimpleUI" will currently break the fiwikis page layout as it will collide with some templates.

Huwiki solves this issue by moving the FlaggedRevs box next to the page title. T197912 asks to have this as a built-in option, but not much happened around it in the past few years. If we want to simplify things, maybe making it the only option is also a possibility.

That will royally break in the new vector where the language links are being put (see https://en.wikipedia.beta.wmflabs.org/wiki/Germany):

image.png (156×976 px, 34 KB)

So one more reason to resolve T197912—if the box’s position is tied to the indicators, wherever they are, it will work well on both new and legacy Vector (although fixing the JavaScript hack seems pretty easy, but it’s still not ideal and works only on huwiki). By the way, is there a wiki with FlaggedRevs on beta cluster, where I could test this change? If not, when is it expected to hit production as part of the (currently opt-in) new Vector? Never mind, I found out that the German one has it.

Hello. Following the message at Georgian wikipedia - ka.wiki - I assume that this ticket should concern us as well.
Right now we have two versions of articles - we call them verified and non-verified. Non-registered users see the verified version by default, and registered see the non-verified version. We also have two kinds of users - people who verify articles, and people who are automatically verified. I would like to receive a confirmation to bring it to the ka.wiki community that this architecture will not be damaged by this change.

Ladsgroup, thank you for looking into this. Should we assume that English Wikinews will likely be affected? As I understand it, we have something similar to what Melberg described above. Cheers.

English Wikinews seems to have "quality" tier (unlike kawiki), Compare https://en.wikinews.org/w/api.php?action=flagconfig and https://ka.wikipedia.org/w/api.php?action=flagconfig and that will be dropped, you will have "unchecked" and "checked" version only, not "quality". Also, some special pages will be dropped but those are useless or has an alternative (like checking history).

Hmm, If I understand this correctly the "sighted" (stable version) is enough for Google News and it doesn't use quality tier for aggregating news. It woud be nice to confirm somehow.

Restricted Application added a subscriber: alaa. · View Herald TranscriptApr 3 2021, 5:33 PM
This comment was removed by Nehaoua.

I'm willing to give fixing some of FlaggedRevs a little if I can wrap my head around the code, and given I have the time for it.

Aklapper removed a subscriber: MichaelSchoenitzer_WMDE.

For everyone's info, currently no Code-Stewardship-Reviews are taking place as there is no clear path forward and as this is not prioritized work.
(Entirely personal opinion: I also assume lack of decision authority due to WMF not having a CTO currently. However, discussing this is off-topic for this task.)

Hmm, If I understand this correctly the "sighted" (stable version) is enough for Google News and it doesn't use quality tier for aggregating news. It woud be nice to confirm somehow.

That's correct.

Google news will use the feeds on the Wikinews main page (With the ?dplid url parameter as they need a number in them). Historically the goal was for them to use https://en.wikinews.org/wiki/Special:NewsFeed but i don't think they actually do (Not sure).

Either way, nobody on enwikinews uses multi tier quality for anything.

wgFlaggedRevsLowProfile is also set to true everywhere except huwiki and ptwikisource.

Also, on that wiki, wgSimpleFlaggedRevsUI is enabled, so the visual difference would be very minor (the little icon with hover info only shows when there is some revision pending review (directly or indirectly via inclusion) . Maybe it could be announce on some village pump page there and removed?