Page MenuHomePhabricator

Mark patrols by rollback as "manually patrolled" instead of "autopatrolled"
Open, Needs TriagePublic

Description

When rollback is used to revert an unpatrolled revision, the "rc_patrolled" field of the reverted revision in the recentchanges table is currently updated from value 0 (meaning "unpatrolled") to value 2 (meaning "autopatrolled"). Since the rollback action is usually the result of a manual review process, I think it is much more appropriate to switch to value 1 (meaning "manually patrolled") instead which is also used when a revision is patrolled without rollback.

In fact, value 2 (meaning "autopatrolled") should be reserved for revisions that have been made by editors with the "autopatrol" right at the time when the revision was made.

Instructions: In RollbackPage.php use PRC_PATROLLED instead of PRC_AUTOPATROLLED and update tests/phpunit/integration/includes/page/RollbackPageTest.php as necessary.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Tgr added a project: good first task.
Tgr added a subscriber: Tgr.

Thanks for the suggestion! Sounds like a reasonable change.
No one is actively working on patrol functionality at the moment but this seems easy enough for an interested volunteer to do.

Hey, I would love to work on this Issue if possible.

Hi @Homewardgamer! The constants mentioned by @MisterSynergy are RecentChange::PRC_UNPATROLLED / RecentChange::PRC_PATROLLED / RecentChange::PRC_AUTOPATROLLED. An IDE with decent static analysis capabilities will tell you where those constants are used, you just need to find the place that's rollback-related.

https://www.mediawiki.org/wiki/How_to_become_a_MediaWiki_hacker might be helpful if you are new to MediaWiki development.

Hi there, I would like to work on this issue, if the other person hasn't contributed yet.

Anyone is very welcome to provide a patch in Gerrit - thanks in advance!

Hi @Matej_Orlicky I made the changes as you suggested in both RollbackPage.php and tests/phpunit/integration/includes/page/RollbackPageTest.php, but running phpunit test failed with an error.

Here is the error message:

php phpunit.php integration/includes/page/RollbackPageTest.php 
Using PHP 7.2.34-18+0~20210223.60+debian10~1.gbpb21322+wmf2
PHPUnit 8.5.25 #StandWithUkraine

......F........                                                   15 / 15 (100%)

Time: 1.95 seconds, Memory: 42.25 MB

There was 1 failure:

1) MediaWiki\Tests\Page\RollbackPageTest::testRollback
rc_patrolled
Failed asserting that '2' matches expected 1.

/var/www/html/w/tests/phpunit/integration/includes/page/RollbackPageTest.php:180
/var/www/html/w/tests/phpunit/MediaWikiIntegrationTestCase.php:457

Could you provide some suggestions here? I'm familiar with PHP but not all that familiar with the testing portion.

This comment was removed by matej_suchanek.

Could you provide some suggestions here? I'm familiar with PHP but not all that familiar with the testing portion.

Apparently, the test suite needs to be updated as well. The test correctly asserts that the reverted revisions are "autopatrolled", but this is what is supposed to change.

In fact not. The test actually asserts the status of the newest revision, not the reverted one(s). So line 178 should not be changed.
The test should probably assert the status of the reverted one(s) anyways.

It makes me wonder now whether all reverted changes should be marked as manually patrolled if they were autopatrolled...

It makes me wonder now whether all reverted changes should be marked as manually patrolled if they were autopatrolled...

Why so?

The rc_patrolled status of autopatrolled edits (i.e. rc_patrolled=2) should never be changed. These edits have been made by experienced users, thus they do not need to show up in the pipeline of patrollers. 0 -> 1 should be the only possible path.

It makes me wonder now whether all reverted changes should be marked as manually patrolled if they were autopatrolled...

Why so?

The rc_patrolled status of autopatrolled edits (i.e. rc_patrolled=2) should never be changed. These edits have been made by experienced users, thus they do not need to show up in the pipeline of patrollers. 0 -> 1 should be the only possible path.

I'm describing what the code does now / would do and I wonder if it really should...

Could you provide some suggestions here? I'm familiar with PHP but not all that familiar with the testing portion.

Apparently, the test suite needs to be updated as well. The test correctly asserts that the reverted revisions are "autopatrolled", but this is what is supposed to change.

In fact not. The test actually asserts the status of the newest revision, not the reverted one(s). So line 178 should not be changed.
The test should probably assert the status of the reverted one(s) anyways.

It makes me wonder now whether all reverted changes should be marked as manually patrolled if they were autopatrolled...

The above was resolved in rMW5e5c879be276: RollbackPage: Make rollback not overwrite manual RC patrol status.
If appropriate, the requested change should now be trivial.