From b5c22f2b779f32ca0642d65cdb8b0c640be990b2 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Sat, 19 Feb 2022 14:49:58 +0100 Subject: [PATCH] Improve wording for throttled filter warnings List which actions were disabled, or explicitly say that no actions were disabled if that's the case. Also avoid the word "throttle" in messages as it may be hard to translate. Also don't suggest optimizations to the filter conditions -- unoptimized rules have nothing to do with a filter being throttled. Bug: T200036 Change-Id: Id989fb185453d068b7685241ee49189a2df67b5f --- i18n/en.json | 7 +++-- i18n/qqq.json | 7 +++-- includes/EchoNotifier.php | 22 +++++++++++---- includes/ServiceWiring.php | 1 + includes/SpecsFormatter.php | 17 +++++++++++ includes/ThrottleFilterPresentationModel.php | 23 ++++++++++++++- includes/View/AbuseFilterViewEdit.php | 28 +++++++++---------- .../phpunit/integration/EchoNotifierTest.php | 24 ++++++++++++---- 8 files changed, 100 insertions(+), 29 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 10ff93235..ee4f213fe 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -145,7 +145,7 @@ "abusefilter-enabled": "Enabled", "abusefilter-deleted": "Deleted", "abusefilter-disabled": "Disabled", - "abusefilter-throttled": "throttled", + "abusefilter-throttled": "High rate of matches", "abusefilter-hitcount": "$1 {{PLURAL:$1|hit|hits}}", "abusefilter-new": "Create a new filter", "abusefilter-import-button": "Import filter", @@ -192,7 +192,8 @@ "abusefilter-edit-oldwarning-view": "You are viewing an old version of this filter.\nThe statistics quoted are for the most recent version of the filter. •\n[[Special:AbuseFilter/history/$2|Return to this filter's history]].", "abusefilter-edit-status-label": "Statistics:", "abusefilter-edit-status": "Of the last $1 {{PLURAL:$1|action|actions}}, this filter has matched $2 ($3%).\nOn average, its run time is $4 ms, and it consumes $5 {{PLURAL:$5|condition|conditions}} of the condition limit.", - "abusefilter-edit-throttled-warning": "'''Warning:''' This filter was automatically flagged as harmful. As a safety measure, the following actions will not execute ($1). Please review and [[mw:Extension:AbuseFilter/Conditions|optimize]] your conditions to remove this restriction", + "abusefilter-edit-throttled-warning": "'''Warning:''' This filter was automatically flagged as harmful. As a safety measure, the following {{PLURAL:$2|action|actions}} will not execute: $1. Please check whether the high rate of matches is expected. If so, you can save the filter again to remove this restriction.", + "abusefilter-edit-throttled-warning-no-actions": "'''Warning:''' This filter was automatically flagged as harmful. No actions were automatically disabled, but please check whether the high rate of matches is expected. If so, you can ignore this warning.", "abusefilter-edit-new": "New filter", "abusefilter-edit-save": "Save filter", "abusefilter-edit-id": "Filter ID:", @@ -573,6 +574,8 @@ "tag-abusefilter-condition-limit": "condition limit reached", "tag-abusefilter-condition-limit-description": "Edits or other events that couldn't be checked by all active [[Special:AbuseFilter|abuse filters]] ([[mw:Extension:AbuseFilter/Conditions|help]]).", "notification-header-throttle-filter": "Abuse filter $2 {{GENDER:$1|you}} recently edited was throttled.", + "notification-header-throttle-filter-actions": "The abuse filter $2 {{GENDER:$1|you}} recently edited had a high rate of matches and the following {{PLURAL:$4|action was|actions were}} automatically disabled: $3.", + "notification-header-throttle-filter-no-actions": "The abuse filter $2 {{GENDER:$1|you}} recently edited had a high rate of matches but no actions were automatically disabled.", "notification-subject-throttle-filter": "An abuse filter {{GENDER:$1|you}} edited was throttled on {{SITENAME}}", "notification-link-text-show-filter": "Show filter" } diff --git a/i18n/qqq.json b/i18n/qqq.json index 4d97d591c..c75f99aaf 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -186,7 +186,7 @@ "abusefilter-enabled": "Abuse filter status.\n{{Identical|Enabled}}", "abusefilter-deleted": "Abuse filter status.\n{{Identical|Deleted}}", "abusefilter-disabled": "Abuse filter status.\n{{Identical|Disabled}}", - "abusefilter-throttled": "Abuse filter status where some actions have been automatically disabled. See {{msg-mw|abusefilter-edit-throttled-warning}}", + "abusefilter-throttled": "Abuse filter status where the filter is matching many edits. See {{msg-mw|abusefilter-edit-throttled-warning}} and {{msg-mw|abusefilter-edit-throttled-warning-no-actions}}.", "abusefilter-hitcount": "Indicates number of times an abuse filter was triggered. Parameters:\n* $1 is the number of hits.", "abusefilter-new": "Button text for creating a new abuse filter.", "abusefilter-import-button": "Used as link text in the navigation toolbar.\n\nThe link points to [[Special:AbuseLog]].", @@ -233,7 +233,8 @@ "abusefilter-edit-oldwarning-view": "Warning displayed when viewing an older version of a filter. Parameters:\n* $1 - (Unused) history ID\n* $2 - filter ID\nSee also {{msg-mw|AbuseFilter-edit-oldwarning}}", "abusefilter-edit-status-label": "Field label for abuse filter statistics.\n{{Identical|Statistics}}", "abusefilter-edit-status": "Parameters:\n* $1 - number of actions\n* $2 - matched count\n* $3 - matched percentage\n* $4 - time (in milliseconds)\n* $5 - number of conditions", - "abusefilter-edit-throttled-warning": "Used as warning message when the filter is throttled and actions will not execute. Parameters:\n* $1 - is a string containing the actions that will not execute", + "abusefilter-edit-throttled-warning": "Used as warning message when the filter is throttled and actions will not execute. Parameters:\n* $1 - is a string containing the actions that will not execute\n* $2 - number of disabled actions, for PLURAL", + "abusefilter-edit-throttled-warning-no-actions": "Used as warning message when the filter is throttled but no actions were disabled.", "abusefilter-edit-new": "Field value in case an edited filter is new.", "abusefilter-edit-save": "Submit button text to save a filter.", "abusefilter-edit-id": "Field label for filter identifier.\n{{Identical|Filter ID}}", @@ -614,6 +615,8 @@ "tag-abusefilter-condition-limit": "Change tag for edits that reached the condition limit", "tag-abusefilter-condition-limit-description": "Description for \"condition limit reached\" change tag", "notification-header-throttle-filter": "Header text for a notification when an abuse filter was throttled after the user edited it. Parameters:\n* $1 - the username of the viewing user, for use in GENDER\n* $2 - filter ID", + "notification-header-throttle-filter-actions": "Header text for a notification when an abuse filter was throttled after the user edited it and some actions were automatically disabled. Parameters:\n* $1 - the username of the viewing user, for use in GENDER\n* $2 - filter ID\n* $3 - list of actions that were disabled\n* $4 - number of disabled actions, for PLURAL", + "notification-header-throttle-filter-no-actions": "Header text for a notification when an abuse filter was throttled after the user edited it but no actions were automatically disabled. Parameters:\n* $1 - the username of the viewing user, for use in GENDER\n* $2 - filter ID", "notification-subject-throttle-filter": "Email subject line for email notice when an abuse filter was throttled after the user edited it. Parameters:\n* $1 - the username of the receiving user, for use in GENDER", "notification-link-text-show-filter": "Label for button that links to the filter that was throttled." } diff --git a/includes/EchoNotifier.php b/includes/EchoNotifier.php index d57241626..9abb64870 100644 --- a/includes/EchoNotifier.php +++ b/includes/EchoNotifier.php @@ -3,6 +3,8 @@ namespace MediaWiki\Extension\AbuseFilter; use EchoEvent; +use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesRegistry; +use MediaWiki\Extension\AbuseFilter\Filter\ExistingFilter; use MediaWiki\Extension\AbuseFilter\Special\SpecialAbuseFilter; use Title; @@ -16,19 +18,23 @@ class EchoNotifier { /** @var FilterLookup */ private $filterLookup; - + /** @var ConsequencesRegistry */ + private $consequencesRegistry; /** @var bool */ private $isEchoLoaded; /** * @param FilterLookup $filterLookup + * @param ConsequencesRegistry $consequencesRegistry * @param bool $isEchoLoaded */ public function __construct( FilterLookup $filterLookup, + ConsequencesRegistry $consequencesRegistry, bool $isEchoLoaded ) { $this->filterLookup = $filterLookup; + $this->consequencesRegistry = $consequencesRegistry; $this->isEchoLoaded = $isEchoLoaded; } @@ -42,10 +48,10 @@ class EchoNotifier { /** * @param int $filter - * @return int + * @return ExistingFilter */ - private function getLastUserIDForFilter( int $filter ): int { - return $this->filterLookup->getFilter( $filter, false )->getUserID(); + private function getFilterObject( int $filter ): ExistingFilter { + return $this->filterLookup->getFilter( $filter, false ); } /** @@ -54,11 +60,17 @@ class EchoNotifier { * @return array */ public function getDataForEvent( int $filter ): array { + $filterObj = $this->getFilterObject( $filter ); + $throttledActionNames = array_intersect( + $filterObj->getActionsNames(), + $this->consequencesRegistry->getDangerousActionNames() + ); return [ 'type' => self::EVENT_TYPE, 'title' => $this->getTitleForFilter( $filter ), 'extra' => [ - 'user' => $this->getLastUserIDForFilter( $filter ), + 'user' => $filterObj->getUserID(), + 'throttled-actions' => $throttledActionNames, ], ]; } diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index fa9aec2c7..69b10ed54 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -150,6 +150,7 @@ return [ EchoNotifier::SERVICE_NAME => static function ( MediaWikiServices $services ): EchoNotifier { return new EchoNotifier( $services->getService( FilterLookup::SERVICE_NAME ), + $services->getService( ConsequencesRegistry::SERVICE_NAME ), ExtensionRegistry::getInstance()->isLoaded( 'Echo' ) ); }, diff --git a/includes/SpecsFormatter.php b/includes/SpecsFormatter.php index 49a418e42..1da2195a0 100644 --- a/includes/SpecsFormatter.php +++ b/includes/SpecsFormatter.php @@ -4,7 +4,9 @@ namespace MediaWiki\Extension\AbuseFilter; use Language; use MediaWiki\Extension\AbuseFilter\Filter\AbstractFilter; +use Message; use MessageLocalizer; +use RawMessage; /** * @todo Improve this once DI around Message objects is improved in MW core. @@ -32,6 +34,7 @@ class SpecsFormatter { /** * @param string $action * @return string HTML + * @todo Replace usage with getActionMessage */ public function getActionDisplay( string $action ): string { // Give grep a chance to find the usages: @@ -42,6 +45,20 @@ class SpecsFormatter { return $msg->isDisabled() ? htmlspecialchars( $action ) : $msg->escaped(); } + /** + * @param string $action + * @return Message + */ + public function getActionMessage( string $action ): Message { + // Give grep a chance to find the usages: + // abusefilter-action-tag, abusefilter-action-throttle, abusefilter-action-warn, + // abusefilter-action-blockautopromote, abusefilter-action-block, abusefilter-action-degroup, + // abusefilter-action-rangeblock, abusefilter-action-disallow + $msg = $this->messageLocalizer->msg( "abusefilter-action-$action" ); + // XXX Why do we expect the message to be disabled? + return $msg->isDisabled() ? new RawMessage( $action ) : $msg; + } + /** * @param string $action * @param string[] $parameters diff --git a/includes/ThrottleFilterPresentationModel.php b/includes/ThrottleFilterPresentationModel.php index 0b89645cd..a3116cde7 100644 --- a/includes/ThrottleFilterPresentationModel.php +++ b/includes/ThrottleFilterPresentationModel.php @@ -3,6 +3,7 @@ namespace MediaWiki\Extension\AbuseFilter; use EchoEventPresentationModel; +use Message; class ThrottleFilterPresentationModel extends EchoEventPresentationModel { @@ -19,7 +20,27 @@ class ThrottleFilterPresentationModel extends EchoEventPresentationModel { public function getHeaderMessage() { $text = $this->event->getTitle()->getText(); list( , $filter ) = explode( '/', $text, 2 ); - return $this->msg( 'notification-header-throttle-filter' ) + $disabledActions = $this->event->getExtraParam( 'throttled-actions' ); + if ( $disabledActions === null ) { + // BC for when we didn't include the actions here. + return $this->msg( 'notification-header-throttle-filter' ) + ->params( $this->getViewingUserForGender() ) + ->numParams( $filter ); + } + if ( $disabledActions ) { + $specsFormatter = AbuseFilterServices::getSpecsFormatter(); + $specsFormatter->setMessageLocalizer( $this ); + $disabledActionsLocalized = []; + foreach ( $disabledActions as $action ) { + $disabledActionsLocalized[] = $specsFormatter->getActionMessage( $action )->text(); + } + return $this->msg( 'notification-header-throttle-filter-actions' ) + ->params( $this->getViewingUserForGender() ) + ->numParams( $filter ) + ->params( Message::listParam( $disabledActionsLocalized ) ) + ->params( count( $disabledActionsLocalized ) ); + } + return $this->msg( 'notification-header-throttle-filter-no-actions' ) ->params( $this->getViewingUserForGender() ) ->numParams( $filter ); } diff --git a/includes/View/AbuseFilterViewEdit.php b/includes/View/AbuseFilterViewEdit.php index 656344730..4ad9265f0 100644 --- a/includes/View/AbuseFilterViewEdit.php +++ b/includes/View/AbuseFilterViewEdit.php @@ -395,26 +395,26 @@ class AbuseFilterViewEdit extends AbuseFilterView { } if ( $filterObj->isThrottled() ) { - $throttledActions = array_intersect( + $throttledActionNames = array_intersect( $filterObj->getActionsNames(), $this->consequencesRegistry->getDangerousActionNames() ); - if ( $throttledActions ) { - $throttledActions = array_map( - function ( $filterAction ) { - // TODO: This is SpecsFormatter::getActionDisplay, but not escaped - return $this->msg( 'abusefilter-action-' . $filterAction )->text(); - }, - $throttledActions - ); + if ( $throttledActionNames ) { + $throttledActionsLocalized = []; + foreach ( $throttledActionNames as $actionName ) { + $throttledActionsLocalized[] = $this->specsFormatter->getActionMessage( $actionName )->text(); + } - $flags .= Html::warningBox( - $this->msg( 'abusefilter-edit-throttled-warning' ) - ->plaintextParams( $lang->commaList( $throttledActions ) ) - ->parseAsBlock() - ); + $throttledMsg = $this->msg( 'abusefilter-edit-throttled-warning' ) + ->plaintextParams( $lang->commaList( $throttledActionsLocalized ) ) + ->params( count( $throttledActionsLocalized ) ) + ->parseAsBlock(); + } else { + $throttledMsg = $this->msg( 'abusefilter-edit-throttled-warning-no-actions' ) + ->parseAsBlock(); } + $flags .= Html::warningBox( $throttledMsg ); } foreach ( $checkboxes as $checkboxId ) { diff --git a/tests/phpunit/integration/EchoNotifierTest.php b/tests/phpunit/integration/EchoNotifierTest.php index 98025988e..bdd093ea2 100644 --- a/tests/phpunit/integration/EchoNotifierTest.php +++ b/tests/phpunit/integration/EchoNotifierTest.php @@ -3,6 +3,7 @@ namespace MediaWiki\Extension\AbuseFilter\Tests\Integration; use EchoEvent; +use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesRegistry; use MediaWiki\Extension\AbuseFilter\EchoNotifier; use MediaWiki\Extension\AbuseFilter\Filter\ExistingFilter; use MediaWiki\Extension\AbuseFilter\FilterLookup; @@ -44,11 +45,16 @@ class EchoNotifierTest extends MediaWikiIntegrationTestCase { * @dataProvider provideDataForEvent * @covers ::__construct * @covers ::getDataForEvent - * @covers ::getLastUserIDForFilter + * @covers ::getFilterObject * @covers ::getTitleForFilter */ public function testGetDataForEvent( bool $loaded, int $filter, int $userID ) { - $notifier = new EchoNotifier( $this->getFilterLookup(), $loaded ); + $expectedThrottledActions = []; + $notifier = new EchoNotifier( + $this->getFilterLookup(), + $this->createMock( ConsequencesRegistry::class ), + $loaded + ); [ 'type' => $type, 'title' => $title, @@ -60,7 +66,7 @@ class EchoNotifierTest extends MediaWikiIntegrationTestCase { $this->assertSame( -1, $title->getNamespace() ); [ , $subpage ] = explode( '/', $title->getText(), 2 ); $this->assertSame( (string)$filter, $subpage ); - $this->assertSame( [ 'user' => $userID ], $extra ); + $this->assertSame( [ 'user' => $userID, 'throttled-actions' => $expectedThrottledActions ], $extra ); } /** @@ -70,7 +76,11 @@ class EchoNotifierTest extends MediaWikiIntegrationTestCase { if ( !class_exists( EchoEvent::class ) ) { $this->markTestSkipped( 'Echo not loaded' ); } - $notifier = new EchoNotifier( $this->getFilterLookup(), true ); + $notifier = new EchoNotifier( + $this->getFilterLookup(), + $this->createMock( ConsequencesRegistry::class ), + true + ); $this->assertInstanceOf( EchoEvent::class, $notifier->notifyForFilter( 1 ) ); } @@ -80,7 +90,11 @@ class EchoNotifierTest extends MediaWikiIntegrationTestCase { public function testNotifyForFilter_EchoNotLoaded() { $lookup = $this->createMock( FilterLookup::class ); $lookup->expects( $this->never() )->method( $this->anything() ); - $notifier = new EchoNotifier( $lookup, false ); + $notifier = new EchoNotifier( + $lookup, + $this->createMock( ConsequencesRegistry::class ), + false + ); $this->assertFalse( $notifier->notifyForFilter( 1 ) ); }