From 86d33524645e4b0b1a8e203d76fe289fcfc80382 Mon Sep 17 00:00:00 2001 From: WMDE-Fisch Date: Tue, 5 Jul 2016 16:45:14 +0200 Subject: [PATCH] Refactored generation of mention events. To increase reusability in future changes. Change-Id: Ia56f4c587361f0214d738cdf690cf9abf93a2021 --- includes/DiscussionParser.php | 191 +++++++++++++++---------- tests/phpunit/DiscussionParserTest.php | 111 +++++++++++++- 2 files changed, 229 insertions(+), 73 deletions(-) diff --git a/includes/DiscussionParser.php b/includes/DiscussionParser.php index 129bac0d1..9da9581e1 100644 --- a/includes/DiscussionParser.php +++ b/includes/DiscussionParser.php @@ -147,84 +147,36 @@ abstract class EchoDiscussionParser { } $content = self::stripHeader( $content ); - $output = self::parseNonEditWikitext( $content, new Article( $title ) ); - $links = $output->getLinks(); - - if ( !isset( $links[NS_USER] ) || !is_array( $links[NS_USER] ) ) { + $userLinks = self::getUserLinks( $content, $title ); + if ( !$userLinks ) { return; } - $mentionedUsers = array(); - $unknownUsers = array(); - $anonymousUsers = array(); - $count = 0; - $stats = MediaWikiServices::getInstance()->getStatsdDataFactory(); - foreach ( $links[NS_USER] as $dbk => $page_id ) { - // we should not add user to 'mention' notification list if - // 1. the user link links to a subpage - if ( self::hasSubpage( $dbk ) ) { - continue; - } + $userMentions = self::getUserMentions( $title, $revision->getUser( Revision::RAW ), $userLinks ); + $overallMentionsCount = self::getOverallUserMentionsCount( $userMentions ); + if ( $overallMentionsCount === 0 ) { + return; + } - // 2. user is an anonymous IP - if ( User::isIP( $dbk ) ) { - $anonymousUsers[] = $dbk; - $count++; - $stats->increment( 'echo.event.mention.error.anonUser' ); - continue; - } - - $user = User::newFromName( $dbk ); - // 3. the user name is not valid - if ( !$user ) { - $unknownUsers[] = str_replace( '_', ' ', $dbk ); - $count++; - $stats->increment( 'echo.event.mention.error.invalidUser' ); - continue; - } - // 4. the user mentions themselves - if ( $user->getId() == $revision->getUser( Revision::RAW ) ) { - $stats->increment( 'echo.event.mention.error.sameUser' ); - continue; - } - // 5. the user is the owner of the talk page - if ( $title->getNamespace() === NS_USER_TALK && $title->getDBkey() === $dbk ) { - $stats->increment( 'echo.event.mention.error.ownPage' ); - continue; - } - // 6. user does not exist - if ( $user->getId() === 0 ) { - $unknownUsers[] = str_replace( '_', ' ', $dbk ); - $count++; - $stats->increment( 'echo.event.mention.error.unknownUser' ); - continue; - } - - $mentionedUsers[$user->getId()] = $user->getId(); - $count++; - // If more users are being pinged this is likely a spam/attack vector - // Don't send any mention notifications. - if ( $count > $wgEchoMaxMentionsCount ) { - if ( $wgEchoMentionStatusNotifications ) { - EchoEvent::create( array( - 'type' => 'mention-failure-too-many', - 'title' => $title, - 'extra' => array( - 'max-mentions' => $wgEchoMaxMentionsCount, - 'section-title' => $header, - 'notifyAgent' => true - ), - 'agent' => $agent, - ) ); - } - $stats->increment( 'echo.event.mention.error.tooMany' ); - return; + if ( $overallMentionsCount > $wgEchoMaxMentionsCount ) { + if ( $wgEchoMentionStatusNotifications ) { + EchoEvent::create( array( + 'type' => 'mention-failure-too-many', + 'title' => $title, + 'extra' => array( + 'max-mentions' => $wgEchoMaxMentionsCount, + 'section-title' => $header, + 'notifyAgent' => true + ), + 'agent' => $agent, + ) ); } + return; } if ( $wgEchoMentionStatusNotifications ) { // TODO batch? - foreach ( $unknownUsers as $unknownUser ) { + foreach ( $userMentions['unknownUsers'] as $unknownUser ) { EchoEvent::create( array( 'type' => 'mention-failure', 'title' => $title, @@ -239,7 +191,7 @@ abstract class EchoDiscussionParser { } // TODO batch? - foreach ( $anonymousUsers as $anonymousUser ) { + foreach ( $userMentions['anonymousUsers'] as $anonymousUser ) { EchoEvent::create( array( 'type' => 'mention-failure', 'title' => $title, @@ -254,7 +206,7 @@ abstract class EchoDiscussionParser { } } - if ( !$mentionedUsers ) { + if ( !$userMentions['validMentions'] ) { return; } @@ -265,12 +217,107 @@ abstract class EchoDiscussionParser { 'content' => $content, 'section-title' => $header, 'revid' => $revision->getId(), - 'mentioned-users' => $mentionedUsers, + 'mentioned-users' => $userMentions['validMentions'], ), 'agent' => $agent, ) ); } + private static function getOverallUserMentionsCount( $userMentions ) { + return count( $userMentions, COUNT_RECURSIVE ) - count( $userMentions ); + } + + /** + * @return array[] + * Set of arrays containing valid mentions and possible intended but failed mentions. + * - [validMentions]: An array of valid users to mention with ID => ID. + * - [unknownUsers]: An array of DBKey strings representing unknown users. + * - [validMentions]: An array of DBKey strings representing anonymous IP users. + */ + private static function getUserMentions( Title $title, $revisionUserId, array $userLinks ) { + global $wgEchoMaxMentionsCount; + $userMentions = array( + 'validMentions' => array(), + 'unknownUsers' => array(), + 'anonymousUsers' => array(), + ); + + $count = 0; + $stats = MediaWikiServices::getInstance()->getStatsdDataFactory(); + + foreach ( $userLinks as $dbk => $page_id ) { + // If more users are being pinged this is likely a spam/attack vector + // Don't send any mention notifications. + if ( $count > $wgEchoMaxMentionsCount ) { + $stats->increment( 'echo.event.mention.error.tooMany' ); + break; + } + + // we should not add user to 'mention' notification list if + // 1. the user link links to a subpage + if ( self::hasSubpage( $dbk ) ) { + continue; + } + + // 2. user is an anonymous IP + if ( User::isIP( $dbk ) ) { + $userMentions['anonymousUsers'][] = $dbk; + $count++; + $stats->increment( 'echo.event.mention.error.anonUser' ); + continue; + } + + $user = User::newFromName( $dbk ); + // 3. the user name is not valid + if ( !$user ) { + $userMentions['unknownUsers'][] = str_replace( '_', ' ', $dbk ); + $count++; + $stats->increment( 'echo.event.mention.error.invalidUser' ); + continue; + } + + // 4. the user mentions themselves + if ( $user->getId() === $revisionUserId ) { + $stats->increment( 'echo.event.mention.error.sameUser' ); + continue; + } + + // 5. the user is the owner of the talk page + if ( $title->getNamespace() === NS_USER_TALK && $title->getDBkey() === $dbk ) { + $stats->increment( 'echo.event.mention.error.ownPage' ); + continue; + } + + // 6. user does not exist + if ( $user->getId() === 0 ) { + $userMentions['unknownUsers'][] = str_replace( '_', ' ', $dbk ); + $count++; + $stats->increment( 'echo.event.mention.error.unknownUser' ); + continue; + } + + $userMentions['validMentions'][$user->getId()] = $user->getId(); + $count++; + } + + return $userMentions; + } + + /** + * @return bool|array + * Array of links in the user namespace with DBKey => ID. + */ + private static function getUserLinks( $content, Title $title ) { + $output = self::parseNonEditWikitext( $content, new Article( $title ) ); + $links = $output->getLinks(); + + if ( !isset( $links[NS_USER] ) || !is_array( $links[NS_USER] ) ) { + return false; + } + + return $links[NS_USER]; + } + private static function hasSubpage( $dbk ) { return strpos( $dbk, '/' ) !== false; } diff --git a/tests/phpunit/DiscussionParserTest.php b/tests/phpunit/DiscussionParserTest.php index b1fbd3e9f..e35a01a66 100644 --- a/tests/phpunit/DiscussionParserTest.php +++ b/tests/phpunit/DiscussionParserTest.php @@ -174,6 +174,7 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention', 'agent' => 'Cwobeel', + 'section-title' => 'Grand jury no bill reception', /* * I wish I could also compare EchoEvent::$extra data to * compare user ids of mentioned users. However, due to @@ -196,6 +197,7 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention', 'agent' => 'Schnark', + 'section-title' => 'Echo-Test', ), ), ), @@ -212,6 +214,7 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention', 'agent' => 'PauloEduardo', + 'section-title' => 'Notificações', ), ), ), @@ -247,10 +250,12 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention', 'agent' => 'PatHadley', + 'section-title' => 'Wizardry required', ), array( 'type' => 'edit-user-talk', 'agent' => 'PatHadley', + 'section-title' => 'Wizardry required', ), ), 'precondition' => 'isParserFunctionsInstalled', @@ -268,10 +273,12 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention', 'agent' => 'Kudpung', + 'section-title' => 'Me?', ), array( 'type' => 'edit-user-talk', 'agent' => 'Kudpung', + 'section-title' => 'Me?', ), ), ), @@ -287,10 +294,12 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention', 'agent' => 'Admin', + 'section-title' => 'Hi', ), array( 'type' => 'edit-user-talk', 'agent' => 'Admin', + 'section-title' => 'Hi', ), ), 'precondition' => 'isParserFunctionsInstalled', @@ -306,13 +315,14 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention-failure', 'agent' => 'Admin', + 'section-title' => 'Hello Users', ), array( 'type' => 'mention-failure', 'agent' => 'Admin', + 'section-title' => 'Hello Users', ), ), - ), ); } @@ -338,6 +348,7 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { $events[] = array( 'type' => $event->getType(), 'agent' => $event->getAgent()->getName(), + 'section-title' => $event->getExtraParam( 'section-title' ), ); return false; } @@ -356,6 +367,7 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { array( 'type' => 'mention-failure-too-many', 'agent' => 'Admin', + 'section-title' => 'Hello Users', 'max-mentions' => 5, ), ); @@ -368,6 +380,7 @@ class EchoDiscussionParserTest extends MediaWikiTestCase { $events[] = array( 'type' => $event->getType(), 'agent' => $event->getAgent()->getName(), + 'section-title' => $event->getExtraParam( 'section-title' ), 'max-mentions' => $event->getExtraParam( 'max-mentions' ), ); return false; @@ -1188,6 +1201,102 @@ TEXT $this->assertEquals( 3, EchoDiscussionParser::getSectionCount( $one . $two . $three ) ); } + public function testGetOverallUserMentionsCount() { + $userMentions = array( + 'validMentions' => array( 1 => 1 ), + 'unknownUsers' => array( 'NotKnown1', 'NotKnown2' ), + 'anonymousUsers' => array( '127.0.0.1' ), + ); + + $discussionParser = TestingAccessWrapper::newFromClass( 'EchoDiscussionParser' ); + $this->assertEquals( 4, $discussionParser->getOverallUserMentionsCount( $userMentions ) ); + } + + public function provider_getUserMentions() { + return array( + array( + array( 'NotKnown1' => 0 ), + array( + 'validMentions' => array(), + 'unknownUsers' => array( 'NotKnown1' ), + 'anonymousUsers' => array(), + ), + 1 + ), + array( + array( '127.0.0.1' => 0 ), + array( + 'validMentions' => array(), + 'unknownUsers' => array(), + 'anonymousUsers' => array( '127.0.0.1' ), + ), + 1 + ), + ); + } + + /** + * @dataProvider provider_getUserMentions + */ + public function testGetUserMentions( $userLinks, $expectedUserMentions, $agent ) { + $title = Title::newFromText( 'Test' ); + $discussionParser = TestingAccessWrapper::newFromClass( 'EchoDiscussionParser' ); + $this->assertEquals( $expectedUserMentions, $discussionParser->getUserMentions( $title, $agent, $userLinks ) ); + } + + public function testGetUserMentions_validMention() { + $userName = 'Admin'; + $userId = User::newFromName( $userName )->getId(); + $expectedUserMentions = array( + 'validMentions' => array( $userId => $userId ), + 'unknownUsers' => array(), + 'anonymousUsers' => array(), + ); + $userLinks = array( $userName => $userId ); + $this->testGetUserMentions( $userLinks, $expectedUserMentions, 1 ); + } + + public function testGetUserMentions_ownMention() { + $userName = 'Admin'; + $userId = User::newFromName( 'Admin' )->getId(); + $expectedUserMentions = array( + 'validMentions' => array(), + 'unknownUsers' => array(), + 'anonymousUsers' => array(), + ); + $userLinks = array( $userName => $userId ); + $this->testGetUserMentions( $userLinks, $expectedUserMentions, $userId ); + } + + public function testGetUserMentions_tooManyMentions() { + $userLinks = array( + 'NotKnown1' => 0, + 'NotKnown2' => 0, + 'NotKnown3' => 0, + '127.0.0.1' => 0, + '127.0.0.2' => 0, + ); + + $this->setMwGlobals( array( + // lower limit for the mention-too-many notification + 'wgEchoMaxMentionsCount' => 3 + ) ); + + $title = Title::newFromText( 'Test' ); + $discussionParser = TestingAccessWrapper::newFromClass( 'EchoDiscussionParser' ); + $this->assertEquals( 4, $discussionParser->getOverallUserMentionsCount( $discussionParser->getUserMentions( $title, 1, $userLinks ) ) ); + } + + private function generateUserIdsForValidUserMentions( $userMentions ) { + $validMentionsWithIds = array(); + foreach ( $userMentions['validMentions'] as $userName ) { + $userId = User::newFromName( $userName )->getId(); + $validMentionsWithIds[$userId] = $userId; + } + $userMentions['validMentions'] = $validMentionsWithIds; + return $userMentions; + } + protected function isParserFunctionsInstalled() { if ( class_exists( 'ExtParserFunctions' ) ) { return true;