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:
none [bawolff did a quick phab search and did not find any.]

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

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 24 2018, 9:26 PM

I'm not sure if this is the right place to start the discussion about this extension, feel free to close this if it needs more discussion beforehand.

greg added a comment.Jan 24 2018, 10:24 PM

I'm not sure if this is the right place to start the discussion about this extension, feel free to close this if it needs more discussion beforehand.

There's no harm in starting it here. :)

stjn updated the task description. (Show Details)Jan 26 2018, 1:56 PM
stjn added a subscriber: stjn.Jan 26 2018, 1:58 PM

There is no replacement or alternative mapped or planned but one option would be to undeploy this from prod

I don’t think you can say anything like this as simple as that, since the entire workflows of edit reviewing in multiple big projects depend entirely on FlaggedRevs. It can’t be undeployed without any replacement, there is nothing this could bring other than massive outrage.

TheDJ added a subscriber: TheDJ.Jan 28 2018, 10:25 AM

@stjn almost everything we do creates 'massive outrage'. It's important however that options are simply explored, even if they may seem undesirable or possibly even unrealistic.

stjn added a comment.Jan 28 2018, 10:29 AM

@stjn almost everything we do creates 'massive outrage'. It's important however that options are simply explored, even if they may seem undesirable or possibly even unrealistic.

And by commenting I am trying to say that this is not an option worth exploring. Undeploying without anything else instead is as viable of an option like undeploying AbuseFilter is (albeit it, too, has problems with extension support).

Liuxinyu970226 added a subscriber: Liuxinyu970226.EditedFeb 7 2018, 12:17 PM

I vote support to archive, this extension is no longer needed, let's just focus the ORES development trains.

I vote support to archive, this extension is no longer needed, let's just focus the ORES development trains.

ORES have completely different goals - FlaggedRevs allow users to manually flag a version was reviewed and is not a vandalism, while ORES provide predictor giving a score whether it is or isn't a vandalism - but predictor may be wrong in many cases, and if human reviews a revision it should be possible to flag it.
One way to "merge" those extensions would be to extend ORES to handle not just the post-training phase, but also the training/manual classification. AFAIK currently ORES doesn't support it and the training is based on "campaigns/wikilabels" which is NOT an extension and targets a predefined set of edits - not all edits (e.g https://en.wikipedia.org/wiki/Wikipedia:Labels )

@eranroz:

I vote support to archive, this extension is no longer needed, let's just focus the ORES development trains.

ORES have completely different goals - FlaggedRevs allow users to manually flag a version was reviewed and is not a vandalism, while ORES provide predictor giving a score whether it is or isn't a vandalism - but predictor may be wrong in many cases, and if human reviews a revision it should be possible to flag it.
One way to "merge" those extensions would be to extend ORES to handle not just the post-training phase, but also the training/manual classification. AFAIK currently ORES doesn't support it and the training is based on "campaigns/wikilabels" which is NOT an extension and targets a predefined set of edits - not all edits (e.g https://en.wikipedia.org/wiki/Wikipedia:Labels )

PageCuration is likely?

PageCuration is likely?

Yes, though this extension (PageCuration) is enwiki specific and need complete rewrite to be used in other wikis. (T50552)

aaron updated the task description. (Show Details)Feb 17 2018, 9:06 PM
Stryn added a subscriber: Stryn.Mar 6 2018, 7:34 PM
Zache added a subscriber: Zache.Mar 6 2018, 7:43 PM

@stjn almost everything we do creates 'massive outrage'. It's important however that options are simply explored, even if they may seem undesirable or possibly even unrealistic.

And by commenting I am trying to say that this is not an option worth exploring. Undeploying without anything else instead is as viable of an option like undeploying AbuseFilter is (albeit it, too, has problems with extension support).

On dewiki, undeploying AbuseFilter would be honestly much less problematic. Disabling FlaggedRevs just isn't an option there.

Liuxinyu970226 added a comment.EditedMay 18 2018, 11:12 PM

@MGChecker:

On dewiki, undeploying AbuseFilter would be honestly much less problematic. Disabling FlaggedRevs just isn't an option there.

But this can also be a blocker to introduce new features to German e.g. <mapframe> and how will you hope that those should always be fixable?

Anyway, if dewiki really don't wanna uninstall it, what about setting $wgFlaggedRevsOverride to false instead?

@Liuxinyu970226 if I understand correctly it is not just flagged revs which doesn't work but the version history is broken too. It just shows up with FR more easily. So mapframe and graph needs to be fixed anyway.

Tgr added a subscriber: Tgr.May 23 2018, 1:01 PM

FlaggedRevs has two somewhat unrelated sets of funcionality:

  • Allow revisions to be marked as reviewed, so that editors can ensure that every change is reviewed and that editors' time is not wasted by reviewing changes that others have reviewed already. This is sort of a beefed-up version of MW core's patrolling feature, with a persistent storage mechanism for review state (unlike the core feature which is only stored in recentchanges), more functionality (review comments, ability to un-review a change, better revert handling, ability to warn readers of unreviewed pages) and better UX (display review status and reviewer name in the history, special pages for finding various kinds of in-need-of-review content, review statistics).
    • The main problem with it IMO is heavy feature creep: it tried to be too many things and ended up with a sea of confusing and largely undocumented options, many of which no one uses. Most prominently, it was envisioned to provide per-revision PageAssessments-like functionality so you can configure an arbitrary number of review dimensions (such as accuracy, readability...), each with an arbitrary number of review levels, separate user rights requirements for each level of each dimension, and thresholds for how dimension-level combinations should resolve into the three quality-levels that the software makes functional decisions on. All this complexity bloats the code and I am not aware of it being used anywhere in earnest; FlagRev wikis basically just use the extension for a reviewed/not reviewed flag.
    • Also as Amir said the code is outdated and especially our UX paradigms have changed a lot since then, so it is in need of update.
  • To certain readers (e.g. those not logged in), show the last sufficiently well-reviewed revision of a page instead of the current revision. Optionally extend this to templates and files (that is, show the last reviewed version of transcluded templates / files in an article) and integrate that review process (so that there is a way to review the article for changes when the templates have been changed but the article itself has not - although this functionality is presented very confusingly, or maybe it's just broken). Also a management interface to turn on or off this functionality for individual pages.
    • This part is black magic, integrates deeply with the process of parsing (e.g. it creates a second parser cache), in ways that are probably suboptimal today, and does not work with most software that does not use the standard MediaWiki core code path and thus does not get this integration (e.g. T169116, T191585). Probably very few people are able to understand how it works (let alone modify it).

The first set of functionality is IMO far less problematic to maintain than the second and provides clearer benefits. The second is a quality-vs-growth tradeoff (makes pages harder to vandalize, but also makes the wiki less inviting for new editors, who now have to wait for their edits to go through a review queue before appearing); it was (to my knowledge) never seriously measured whether it is a good tradeoff. Nevertheless, both sets of functionality are used heavily by several wikis (including the two largest Wikipedias).

Zache added a comment.May 23 2018, 2:03 PM

beefed-up version of MW core's patrolling feature

I would also add that the one core difference is that with FR users are reviewing changes from last known version to latest and with patrolling reviewers are approving every single revision. For example, with patrolling they are reviewing edits which are already rewritten.

I am not aware of it being used anywhere in earnest

Hewikisource seems to use it, but it may be only WMF wiki that actually uses it.

在T185664#4225282中,@Zache写道:

beefed-up version of MW core's patrolling feature
I would also add that the one core difference is that with FR users are reviewing changes from last known version to latest and with patrolling reviewers are approving every single revision. For example, with patrolling they are reviewing edits which are already rewritten.
I am not aware of it being used anywhere in earnest
Hewikisource seems to use it, but it may be only WMF wiki that actually uses it.

Not always, Wikidata users also use it dynamically, they even opened the function to everyone that "restored to the revision XXX by User:foo"

Added @kaldari and @Catrope as subscribers per Ryan's request.

Cirdan added a subscriber: Cirdan.May 31 2018, 9:54 AM
Krinkle updated the task description. (Show Details)Jul 11 2018, 3:59 AM
Krinkle updated the task description. (Show Details)Jul 11 2018, 4:03 AM

@Jrbranaa What does the "Done" column represent on the Code-Stewardship-Reviews workboard?

@Krinkle, it's supposed to mean that a Code Steward has been found/assigned. In this case it was the Collaboration team. Which is now the Growth team if I understand correctly.

@Krinkle, it's supposed to mean that a Code Steward has been found/assigned. In this case it was the Collaboration team. Which is now the Growth team if I understand correctly.

Okay, that makes sense :)

As a bystander, it looked like Greg only CC-ed the Collaboration team based on the Maintainers page, where Collaboration had been listed as maintainer since before this task was created. I couldn't find in the page history, on this task, or in meeting notes that Collaboration actually decided on it. - or whether it may've been a misunderstanding. I guess it was discussed offline? Anyhow, if so, I suppose this ticket can be closed :)

Yeah, not a stellar job of documentation :-) I'll do a better job next time. I do want to followup to make sure that this made it through the change from Collaboration to Growth.

Liuxinyu970226 added a comment.EditedNov 6 2018, 1:36 PM

@Jrbranaa So we should alter the Herald H26 to also automatically add Growth-Team for MediaWiki-extensions-FlaggedRevs tasks?

(currently that is maintained by @Catrope)

@Liuxinyu970226, yes that makes sense.

I'm not really excited about the Growth team being put on the hook to maintain yet another unloved extension. Of all the patches merged into FlaggedRevs in the past 18 months, only 2 were by a Growth team member (both by me). It's also not very related to our team's mission. @Tgr 's comment identifies two parts, and the first part (reviewing) is related to the old Collaboration team's mission, but the second part (displaying old revisions and doing black magic in the parser) isn't at all. Perhaps this would have a better home in the Reading Infrastructure team, or another team that more regularly works on the reading UI and the parser.

@Catrope, we can certainly find another home for this extension if you think another team is a better fit. Out of curiosity, is this related at all to the change from Collaboration to Growth? I'm trying to get a sense of whether the wrong call was made when we first reviewed this extension for Code Stewardship or if things just changed since then.

So the questions changed to one: Do we need to restructure a purely new "Collaboration team" that unrelated to "Growth team" at all?

@Liuxinyu970226: You are free to start a "Collaboration team", whoever your "we" is. I do not consider that question helpful here though.

94rain added a subscriber: 94rain.May 28 2019, 2:38 PM

@Catrope, we can certainly find another home for this extension if you think another team is a better fit. Out of curiosity, is this related at all to the change from Collaboration to Growth? I'm trying to get a sense of whether the wrong call was made when we first reviewed this extension for Code Stewardship or if things just changed since then.

I think it was somewhat defensible to assign FlaggedRevs to Collaboration because it sort of fit with that team's mission. But it doesn't fit with Growth's mission. We do maintain things that Collaboration actively worked on, but that's typically because the engineers who know the most about those extensions are all on the Growth team; that's also not the case for FlaggedRevs.

Jrbranaa triaged this task as Medium priority.Jun 3 2019, 10:53 PM
Jrbranaa removed a project: Growth-Team.

Spoke with @Catrope and @MMiller_WMF re the ownership of this extension. We determined that this extension is outside the scope of the Growth team and would be better suited somewhere else. As such, this extension has returned to orphan status.

Jrbranaa raised the priority of this task from Medium to High.Jul 16 2019, 9:25 PM
Jrbranaa moved this task from In Review to Prioritized on the Code-Stewardship-Reviews board.
Tgr added a comment.Oct 3 2019, 8:56 PM

Although as noted there it is likely to be a bug with Wikimedia configuration and not the extension itself. (Not sure if *that* has an owner...)

Krinkle renamed this task from FlaggedRevs: code stewardship review to Code stewardship review: FlaggedRevs.Oct 3 2019, 10:16 PM

The extention page states that its authors are Aaron Schulz and Jörg Baach. It may worth to try to contact them because they may know something that only authors know (some hidden deep debugging mode to launch in case of troubles and the like).

I wrote to both of them informing about T185664 and T233561

Bawolff updated the task description. (Show Details)Oct 7 2019, 7:04 AM

FlaggedRevs has two somewhat unrelated sets of functionality: <...>

Yes, and both of them have nothing to do with the FlaggedRevs initial purpose.

It also introduced a set of bizarre things into Wikimedia interfaces. Like say putting all its current and future text editors into WYSIWYNG mode (What You See is What You Never Get). So a visitor may want to change "Jhon Dow" to "John Dow" (stable version) - but in edit mode he will found at exacly same place "Jane Roe" (current version).

The sad things is that FlaggedRevs seems auto-assigned itself as Core feature. So this task is "de jure" open but "de facto" years ago closed with "assigned to Core team" I mean that if for instance English Wikipedia looses ability to have "stabilized" living people articles or "patrol" them - it will be "Unfix Now!" rollback. Even if it restores most ugly bugs or kills most beautiful new feature. That seems like Core to me.

So even without assigning it to Core - it would be useful to unclude FlaggedRevs testing in all deployment tests. T234743 - which is most probably caused by the last (Sep.16-18) activity at T223602 - seems to prove my idea.

At risk of saying or repeating the obvious, FlaggedRevs needs a maintainer. It is a Wikimedia-deployed extension, and it is one which I'd categorise as critical in the maintenance/anti-vandalism sense of the word (as AbuseFilter, CheckUser, etc.). Would Core Platform Team be interested in adopting this?

Zache added a comment.Thu, Nov 28, 3:22 PM

Autopromote is broken for FlaggedRevs, so it is one high priority bug more. T237191

Tgr added a comment.Thu, Nov 28, 11:10 PM

FWIW neither of the current issues (T237191: FlaggedRevs: Automatic user promotion stopped working in dewiki on June 24 and T234743: User rights validation is sometimes malfunctioning (with FlaggedRevs only?)) are problems with FlaggedRevs itself, they are site configuration bugs. Site confiugration does have a steward.