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 around seven years ago 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 out-dated but any (of my) attempts to to modernize it failed due to out-of-date and non-standard php code.

Current developers/maintainers:
None

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)?

No, 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 and PHP fatals (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 45 Wikimedia projects.

Changes committed in last 1, 3, 6, and 12 months
?

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?
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

Event Timeline

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

Is there a default result for an extension if no team agrees to be responsible for it? Undeployment?

Also, just came across https://bash.toolforge.org/quip/AU75enyk6snAnmqnLEnQ

<Krinkle>  Who's responsible for FlaggedRevs these days? Should I delegate via Dan Garry?
<greg-g>  "responsible" is such a strong word

If communities want to continue to use where they should contact to get some of the teams to be responsible? (afaik this is the current situation as there is no replacement and the communities like it as a tool)

Ie if teams cant decide by themselves who should be doing it then it should be contacted to the upper level in the management in the hierarchy where the decision of who is the responsible team should be made.

Is there a default result for an extension if no team agrees to be responsible for it? Undeployment?

No, the default "result" is to forward this to C levels of WMF for a decision. See https://www.mediawiki.org/wiki/Code_stewardship_reviews#FAQ. Per my understanding, this gives one more chance to the extension to be adopted (and funded accordingly).

Is there a default result for an extension if no team agrees to be responsible for it? Undeployment?

No, the default "result" is to forward this to C levels of WMF for a decision. See https://www.mediawiki.org/wiki/Code_stewardship_reviews#FAQ. Per my understanding, this gives one more chance to the extension to be adopted (and funded accordingly).

So should we forward this to C levels? This task was opened in January 2018, with no end in sight and no one (so far) volunteering to be responsible for it

Is there a default result for an extension if no team agrees to be responsible for it? Undeployment?

No, the default "result" is to forward this to C levels of WMF for a decision. See https://www.mediawiki.org/wiki/Code_stewardship_reviews#FAQ. Per my understanding, this gives one more chance to the extension to be adopted (and funded accordingly).

So should we forward this to C levels? This task was opened in January 2018, with no end in sight and no one (so far) volunteering to be responsible for it

To be honest, none of code stewardship requests I have ever filled got anywhere neither most of CSRs in general. I'm losing my hope to the whole process altogether.

So... what's going on with this task (and FlaggedRevs in general)?

Trying to debug T233561: Pending changes: autoreview randomly fails and T275322: Some edits made by extended confirmed users are no longer automatically accepted, no success after an hour. This extension is basically re-implementing significant parts of mediawiki core for reasons unknown to me. I highly suggest just dropping things from it. Most of the code is not even used in production anymore (like quality scale, etc. tags like pristine, trusted, etc.)

Sorry for the delay in response on this task. The Code Stewardship Review process has definitely run into some snags over the course of the past year as there's been a reduction in our capacity and as a result, an inability to find Code Stewards for many of the outstanding items. That being said, there's been a significant amount of activity and discussion around the topic of Code Stewardship as of late and we will be discussing all of existing orphaned code during our annual planning process over the course of the next several weeks. As a result, I hope to have a path forward for this and other orphaned code in the not-to-distant future.

It seems to me that already in advance, we can start discussing the creation of a new extension to replace this: T277411: Create replacement for FlaggedRevs extension.

One point, I have started dropping lots of code that is not enabled in production (like multiple dimensions in tagging) in hope to make its logic simpler and easier to maintain. Some already got merged.

I have started dropping lots of code that is not enabled in production (like multiple dimensions in tagging)

Multiple dimensions are in use on hewikisource. I agree they should be dropped, but it needs communication and maybe some sort of a migration process.
Also a warning for third parties (so interested maintainers can fork) would be nice.

Yes. I haven't removed it yet. So far mostly dead code. Unsupported upgrade paths, unused hooks, etc. but I will drop multiple dimensions soon. Let me write a detailed comment on this.

@Ladsgroup just some notes:

  • "Reviewer" user group is enabled in finnish wikipedia. I think it is ok to drop it from code from fiwiki point of view, but it would be nice to write prior message to the community before removing it ( I can write the message). Afaik, fiwiki is only wikimedia wiki which is using both editor and reviewer user levels.
  • Removing multiple dimensions is propably a good idea. Though i would keep single dimension with multiple levels and ability to set quality/pristine levels as it feels like useful feature, but I am not sure how easy it is to axe dimensions and keep multiple levels (vs. axing it just to unstable/stable revisions)

I know ruWP used them experimentally for a time and should still have those user groups, so you should look into it. It never was meant for wide use though.

I use FlaggedRevs on enWiki and would like to see it retained as a protection option, so I plan to lend a hand to make that happen. I won't be able to fix much of the high-level stuff, but hopefully working on some of the smaller tasks will free up more experienced developers for the critical tasks.

One issue I ran into as a relatively new contributor wanting to help out was the workboard. The open tasks were just one list of nearly 200 (largely untriaged) tasks. Finding something where I could help out was difficult, and the volume masks a lot of related or duplicate tasks. I've started organizing the bugs so that finding tasks will be easier and hopefully easier for newcomers. Right now I've made three columns:

  • UI/UX errors -- editor- and reader-facing problems that don't seem to implicate major code changes with the hope of drawing the attention of new coders (I plan to start with some of these)
  • Feature requests -- new behavior requested by wikis. Even if they don't get implemented soon, they might be helpful for understanding how projects use or would like to use FlaggedRevs
  • Under review -- tasks that need developer eyes to move the task along

Hopefully the efforts were more helpful than harmful, but do let me know if I've overstepped.

I went down a terrible rabbit hole yesterday. Deleted more than 1k lines of code. All dead code: Upgrades from unsupported versions, maintenance scripts that doesn't make any sense to keep around anymore, Hooks it introduced and is never got used, "tone" and "depth" dimensions that are not enabled anywhere, tier3 ("pristine") that's also not enabled anywhere in production. There are way more we can drop.

The problem I see with FlaggedRevs has two aspects:

  • Internal complexities. Bear with me, it's going to get a bit complicated. FlaggedRevs supports multiple dimensions each can have several levels, and multiple "tier"s in each dimension. Meaning I can lock a page to show the stable version to users if reviewers says "accuracy" of the revision is at least level 3 (at least level 3, is called "tier3" or "pristine") and its "depth" is at least 2 (and "tone" is at least level 1 which translates to tier2 or quality and tier1 or checked respectively) and have different tiers in another page.
    • This four dimensional chess is not enabled in production, only one wiki has more than one dimension (Hebrew Wikisource) and only a couple of wikis use more than one tier (Finnish Wikipedia, English Wikibooks, etc.) and even in those wikis, it's not widely used as it's inherently complex.
  • Functional complexities: It introduces 18 new special pages, that's crazy. WikibaseRepo is the biggest extension I can think of and it basically transforms a mediawiki into something else and it has only 30 special pages. Most extension don't introduce more than five special pages (Not to mention this is after lots of clean up). There is similar issue with other parts of the code.

As long as it's like this, any improvements (I did some UI improvements, like changing the icons) or simply maintaining the extension will be a huge pain to any team or anyone (that's why teams are reluctant to take it over). My suggestion:

  • Disable all extra dimensions in production, meaning only Hebrew Wikisource. Possibly with an announcement beforehand.
  • Announcement for 3rd party users about this change.
  • Simplify its logic, unify naming of the dimension (some places it's "accuracy", some places it's "status", my suggestion is "status") and drop code.
  • Disable extra tiers (only one tire: "checked") in production wikis too.
  • Simplify its logic again.
  • Drop most of barely used functionalities. Like most of those special pages.
  • Drop now-unused code. Basically hollow out the extension.
  • Start improving and modernizing it (the UI, tests, namespacaziation, dropping usage of action=ajax, etc.)
  • Hand it over to anyone who wishes to maintain it (I can do it but not at its current shape).

I add user notice as this is going affect communities. Would that sound good to people?

It's a good idea. There was a project in ruwiki to use 2-3 tiers to extended quality check, this was tested during several years, but ended because everyone stopped doing this unhelpful activity. Recently a discussion was held in ruwiki, as a result of which it was decided to not use 2-3 tiers.

This sounds like the best long-term solution. How should we coordinate moving forward with the first stages of refactoring?

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:

(logged-out, no review rights)

(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):

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.