Link directly to the edited section from edit-user-talk events

Adjusted the edit-user-talk event creation to detect and record which section
of the talk page was edited. Flyout, special page, and email messages have
been adjusted to use this section title as a URL fragment when available.

Bug: 46937
Change-Id: I161e2ffda2f2540f64de90cc621fb3b69479d0db

Echo.i18n.php

@@ -63,8 +63,8 @@ $messages['en'] = array(
 
 	// Notification
 	'echo-quotation-marks' => '"$1"',
-	'notification-edit-talk-page2' => '[[User:$1|$1]] {{GENDER:$1|posted}} on your [[User talk:$2|talk page]].',
-	'notification-edit-talk-page-flyout2' => '<b>$1</b> {{GENDER:$1|posted}} on your [[User talk:$2|talk page]].',
+	'notification-edit-talk-page2' => '[[User:$1|$1]] {{GENDER:$1|posted}} on your [[User talk:$2#$3|talk page]].',
+	'notification-edit-talk-page-flyout2' => '<b>$1</b> {{GENDER:$1|posted}} on your [[User talk:$2#$3|talk page]].',
 	'notification-page-linked' => '[[:$2]] was {{GENDER:$1|linked}} from [[:$3]]: [[Special:WhatLinksHere/$2|See all links to this page]]',
 	'notification-page-linked-flyout' => '<b>$2</b> was {{GENDER:$1|linked}} from <b>$3</b>: [[Special:WhatLinksHere/$2|See all links to this page]]',
 	'notification-add-comment2' => '[[User:$1|$1]] {{GENDER:$1|commented}} on "[[$3|$2]]" on the "$4" talk page',
@@ -280,12 +280,14 @@ Parameters:
 	'notification-edit-talk-page2' => "Format for displaying notifications of a user talk page being edited
 * $1 is the username of the person who edited, plain text. Can be used for GENDER.
 * $2 is the current user's name, used in the link to their talk page.
+* $3 is the section title of the discussion, if any, used in the link to their talk page
 See also:
 * {{msg-mw|Notification-edit-talk-page-flyout2}}
 * {{msg-mw|Notification-add-talkpage-topic2}}",
 	'notification-edit-talk-page-flyout2' => "Flyout-specific format for displaying notifications of a user talk page being edited
 * $1 is the username of the person who edited, plain text. Can be used for GENDER.
 * $2 is the current user's name, used in the link to their talk page.
+* $3 is the section title of the discussion, if any, used in the link to their talk page
 See also:
 * {{msg-mw|Notification-edit-talk-page2}}
 * {{msg-mw|Notification-add-talkpage-topic2}}",

Echo.php

@@ -377,12 +377,12 @@ $wgEchoNotifications = array(
 		'bundle' => array( 'web' => true, 'email' => false ),
 		'formatter-class' => 'EchoEditFormatter',
 		'title-message' => 'notification-edit-talk-page2',
-		'title-params' => array( 'agent', 'user' ),
+		'title-params' => array( 'agent', 'user', 'subject-anchor' ),
 		'bundle-message' => 'notification-edit-talk-page-bundle',
 		'bundle-params' => array( 'agent', 'user', 'agent-other-display', 'agent-other-count' ),
 		'payload' => array( 'summary' ),
 		'flyout-message' => 'notification-edit-talk-page-flyout2',
-		'flyout-params' => array( 'agent', 'user' ),
+		'flyout-params' => array( 'agent', 'user', 'subject-anchor' ),
 		'email-subject-message' => 'notification-edit-talk-page-email-subject2',
 		'email-body-message' => 'notification-edit-talk-page-email-body2',
 		'email-body-params' => array( 'email-intro', 'titlelink', 'summary', 'email-footer' ),

formatters/BasicFormatter.php

@@ -333,6 +333,25 @@ class EchoBasicFormatter extends EchoNotificationFormatter {
 		return EchoDiscussionParser::getTextSnippet( $content, 200 );
 	}
 
+	/**
+	 * Extract the subject anchor (linkable portion of the edited page) from
+	 * the event.
+	 *
+	 * @param $event EchoEvent The event to format the subject anchor of
+	 * @return string The anchor on page, or an empty string
+	 */
+	protected function formatSubjectAnchor( EchoEvent $event ) {
+		global $wgParser;
+
+		$extra = $event->getExtra();
+		if ( empty( $extra['section-title'] ) ) {
+			return '';
+		}
+
+		// Strip out #, keeping # in the i18n message makes it look more clear
+		return substr( $wgParser->guessLegacySectionNameFromWikiText( $extra['section-title'] ), 1 );
+	}
+
 	/**
 	 * Generate links based on output format and passed properties
 	 * $event EchoEvent
@@ -353,8 +372,11 @@ class EchoBasicFormatter extends EchoNotificationFormatter {
 		}
 
 		if ( isset( $props['fragment'] ) ) {
-			$title->setFragment( '#' . $props['fragment'] );
+			$fragment = $props['fragment'];
+		} else {
+			$fragment = $this->formatSubjectAnchor( $event );
 		}
+		$title->setFragment( "#$fragment" );
 
 		if ( $this->outputFormat === 'html' || $this->outputFormat === 'flyout' ) {
 			$class = array();

formatters/CommentFormatter.php

@@ -48,28 +48,14 @@ class EchoCommentFormatter extends EchoEditFormatter {
 	 */
 	protected function processParam( $event, $param, $message, $user ) {
 		$extra = $event->getExtra();
-		if ( $param === 'subject-anchor' ) {
-			global $wgParser;
-			if ( !empty( $extra['section-title'] ) ) {
-				$message->params(
-					// Strip out #, keeping # in the i18n message makes it look more clear
-					substr( $wgParser->guessLegacySectionNameFromWikiText( $extra['section-title'] ), 1 )
-				);
-			} else {
-				$message->params( '' );
-			}
-		} elseif ( $param === 'content-page' ) {
+		if ( $param === 'content-page' ) {
 			if ( $event->getTitle() ) {
 				$message->params( $event->getTitle()->getSubjectPage()->getPrefixedText() );
 			} else {
 				$message->params( '' );
 			}
 		} elseif ( $param === 'subject-link' ) {
-			$prop = array();
-			if ( isset( $extra['section-title'] ) && $extra['section-title'] ) {
-				$prop['fragment'] = $extra['section-title'];
-			}
-			$this->setTitleLink( $event, $message, $prop );
+			$this->setTitleLink( $event, $message );
 		} else {
 			parent::processParam( $event, $param, $message, $user );
 		}

formatters/EditFormatter.php

@@ -9,7 +9,9 @@ class EchoEditFormatter extends EchoBasicFormatter {
 	 * @param $user User
 	 */
 	protected function processParam( $event, $param, $message, $user ) {
-		if ( $param === 'difflink' ) {
+		if ( $param === 'subject-anchor' ) {
+			$message->params( $this->formatSubjectAnchor( $event ) );
+		} elseif ( $param === 'difflink' ) {
 			$eventData = $event->getExtra();
 			if ( !isset( $eventData['revid'] ) ) {
 				$message->params( '' );

formatters/NotificationFormatter.php

@@ -62,7 +62,7 @@ abstract class EchoNotificationFormatter {
 
 	/**
 	 * Set the output format that the notification will be displayed in.
-	 * @param $format string A valid output format (by default, 'text', 'html', and 'email' are allowed)
+	 * @param $format string A valid output format (by default, 'text', 'html', 'flyout', and 'email' are allowed)
 	 * @throws MWException
 	 */
 	public function setOutputFormat( $format ) {

formatters/UserRightsFormatter.php

@@ -22,7 +22,7 @@ class EchoUserRightsFormatter extends EchoBasicFormatter {
 				$list = array();
 
 				foreach ( array( 'add', 'remove' ) as $action ) {
-					if ( $extra[$action] ) {
+					if ( isset( $extra[$action] ) && $extra[$action] ) {
 						// Messages that can be used here:
 						// * notification-user-rights-add
 						// * notification-user-rights-remove

includes/DiscussionParser.php

@@ -84,7 +84,11 @@ abstract class EchoDiscussionParser {
 					EchoEvent::create( array(
 						'type' => 'edit-user-talk',
 						'title' => $title,
-						'extra' => array( 'revid' => $revision->getID(), 'minoredit' => $revision->isMinor() ),
+						'extra' => array(
+							'revid' => $revision->getID(),
+							'minoredit' => $revision->isMinor(),
+							'section-title' => self::detectSectionTitle( $interpretation ),
+						),
 						'agent' => $user,
 					) );
 				}
@@ -92,6 +96,31 @@ abstract class EchoDiscussionParser {
 		}
 	}
 
+	/**
+	 * Attempts to determine what section title the edit was performed under (if any)
+	 *
+	 * @param $interpretation array Results of self::getChangeInterpretationForRevision
+	 * @return string The section title if found otherwise a blank string
+	 */
+	protected static function detectSectionTitle( array $interpretation ) {
+		$header = '';
+		foreach ( $interpretation as $action ) {
+			switch( $action['type'] ) {
+			case 'add-comment':
+				$header = self::extractHeader( $action['full-section'] );
+				break;
+
+			case 'new-section-with-comment':
+				$header = self::extractHeader( $action['content'] );
+				break;
+			}
+			if ( $header ) {
+				return $header;
+			}
+		}
+		return '';
+	}
+
 	/**
 	 * For an action taken on a talk page, notify users whose user pages
 	 * are linked.

tests/NotificationFormatterTest.php

@@ -0,0 +1,132 @@
+
+<?php
+
+class EchoNotificationFormatterTest extends MediaWikiTestCase {
+
+	public static function provider_editUserTalkEmail() {
+		return array(
+			array( '/Main_Page#Section_8/', 'Section 8' ),
+			array( '/Main_Page[^#]/', null ),
+		);
+	}
+
+	/**
+	 * @dataProvider provider_editUserTalkEmail
+	 */
+	public function testEditUserTalkEmailNotificationLink( $pattern, $sectionTitle ) {
+		$event = $this->mockEvent( 'edit-user-talk', array(
+			'section-title' => $sectionTitle,
+		) );
+		$event->expects( $this->any() )
+			->method( 'getTitle' )
+			->will( $this->returnValue( Title::newMainPage() ) );
+
+		$formatted = $this->format( $event, 'email' );
+		$this->assertRegExp( $pattern, $formatted['body'] );
+	}
+
+	public static function provider_editUserTalk() {
+		return array(
+			array( '/[[User talk:[^#]+#moar_cowbell|talk page]]/', 'moar_cowbell', 'text' ),
+			array( '/#moar_cowbell/', 'moar_cowbell', 'html' ),
+			array( '/#moar_cowbell/', 'moar_cowbell', 'flyout' ),
+		);
+	}
+
+	/**
+	 * @dataProvider provider_editUserTalk
+	 */
+	public function testEditUserTalkFlyoutSectionLinkFragment( $pattern, $sectionTitle, $format ) {
+		// Required hack so parser doesnt turn the links into redlinks which contain no fragment
+		LinkCache::singleton()->addGoodLinkObj( 42, Title::newFromText( '127.0.0.1', NS_USER_TALK ) );
+
+		$event = $this->mockEvent( 'edit-user-talk', array(
+			'section-title' => $sectionTitle,
+		) );
+
+		$this->assertRegExp( $pattern, $this->format( $event, $format ) );
+	}
+
+	public function provider_formatterDoesntFail() {
+		// Remove events from this array once they have specific tests for their formatting
+		$untested = array(
+			'welcome' => array(),
+			'reverted' => array(
+				'revid' => 42,
+				'reverted-user-id' => 77,
+				'reverted-revision-id' => 13,
+				'method' => 'undo',
+			),
+			'page-linked' => array(
+				'link-from-namespace' => 0,
+				'link-from-title' => 'Karl Sims',
+			),
+			'mention' => array(
+				'content' => 'lorem ipsum dolar sit amet',
+				'section-title' => 'Zombies',
+				'revid' => 42,
+				'mentionedusers' => array( 101 => 101 ),
+			),
+			'user-rights' => array(
+				'user' => 187,
+				'add' => array( 'aaa', 'bbb' ),
+				'remove' => array( 'other' ),
+			),
+		);
+		$formats = array( 'html', 'flyout', 'email', 'text' );
+		$tests = array();
+		foreach ( $untested as $type => $extra ) {
+			foreach ( $formats as $format ) {
+				// Run tests with blank extra data and with the provided extra data
+				$tests[] = array( $format, $type, $extra );
+				$tests[] = array( $format, $type, array() );
+			}
+		}
+
+		return $tests;
+	}
+
+	/**
+	 * @dataProvider provider_formatterDoesntFail
+	 */
+	public function testFormatterDoesntFail( $format, $type, array $extra ) {
+		$result = $this->format( $this->mockEvent( $type, $extra ), $format );
+
+		// generic assertion, could do better
+		if ( $format === 'email' ) {
+			$this->assertInternalType( 'array', $result );
+			$this->assertCount( 3, $result );
+		} else {
+			$this->assertInternalType( 'string', $result );
+			$this->assertGreaterThan( 0, strlen( $result ) );
+		}
+	}
+
+	protected function format( EchoEvent $event, $format, $type = 'web', array $params = array() ) {
+		global $wgEchoNotifications;
+
+		$params += $wgEchoNotifications[ $event->getType() ];
+		$formatter = EchoNotificationFormatter::factory( $params );
+		$formatter->setOutputFormat( $format );
+
+		return $formatter->format( $event, new User, $type );
+	}
+
+	protected function mockEvent( $type, array $extra = array(), Revision $rev = null ) {
+		$event = $this->getMockBuilder( 'EchoEvent' )
+			->disableOriginalConstructor()
+			->getMock();
+		$event->expects( $this->any() )
+			->method( 'getType' )
+			->will( $this->returnValue( $type ) );
+		$event->expects( $this->any() )
+			->method( 'getExtra' )
+			->will( $this->returnValue( $extra ) );
+		if ( $rev !== null ) {
+			$event->expects( $this->any() )
+				->method( 'getRevision' )
+				->will( $this->returnValue( $rev ) );
+		}
+		return $event;
+	}
+}

tests/TalkPageFunctionalTest.php

@@ -0,0 +1,88 @@
+<?php
+/**
+ * @group DataBase
+ * @group medium
+ */
+class EchoTalkPageFunctionalTest extends ApiTestCase {
+
+	protected $dbr;
+
+	public function setUp() {
+		parent::setUp();
+		$this->dbr = MWEchoDbFactory::getDB( DB_SLAVE );
+	}
+
+	/**
+	 * Creates and updates a user talk page a few times to ensure proper events are
+	 * created. The user performing the edits is self::$users['sysop'].
+	 */
+	public function testAddCommentsToTalkPage() {
+		$editor = self::$users['sysop']->user->getName();
+		$talkPage = self::$users['uploader']->user->getName();
+		// A set of messages which will be inserted
+		$messages = array(
+			'Moar Cowbell',
+			"I can haz test\n\nplz?", // checks that the parser allows multi-line comments
+			'blah blah',
+		);
+
+		$messageCount = 0;
+		$this->assertCount($messageCount, $this->fetchAllEvents() );
+
+		// Start a talkpage
+		$content = "== Section 8 ==\n\n" . $this->signedMessage( $editor, $messages[$messageCount] );
+		$this->editPage( $talkPage, $content, '', NS_USER_TALK );
+
+		// Ensure the proper event was created
+		$events = $this->fetchAllEvents();
+		$this->assertCount(1 + $messageCount, $events, 'After initial edit a single event must exist.'); // +1 is due to 0 index
+		$row = end( $events );
+		$this->assertEquals( 'edit-user-talk', $row->event_type );
+		$this->assertEventSectionTitle( 'Section 8', $row );
+
+		// Add another message to the talk page
+		$messageCount++;
+		$content .= $this->signedMessage( $editor, $messages[$messageCount] );
+		$this->editPage( $talkPage, $content, '', NS_USER_TALK );
+
+		// Ensure another event was created
+		$events = $this->fetchAllEvents();
+		$this->assertCount(1 + $messageCount, $events);
+		$row = end( $events );
+		$this->assertEquals( 'edit-user-talk', $row->event_type );
+		$this->assertEventSectionTitle( 'Section 8', $row );
+
+		// Add a new section and a message within it
+		$messageCount++;
+		$content .= "\n\n== EE ==\n\n" . $this->signedMessage( $editor, $messages[$messageCount] );
+		$this->editPage( $talkPage, $content, '', NS_USER_TALK );
+
+		// Ensure this event has the new section title
+		$events = $this->fetchAllEvents();
+		$this->assertCount(1 + $messageCount, $events);
+		$row = end( $events );
+		$this->assertEquals( 'edit-user-talk', $row->event_type );
+		$this->assertEventSectionTitle( 'EE', $row );
+	}
+
+	protected function assertEventSectionTitle( $sectionTitle, $row ) {
+		$this->assertNotNull( $row->event_extra, 'Event must contain extra data.' );
+		$extra = unserialize( $row->event_extra );
+		$this->assertArrayHasKey( 'section-title', $extra, 'Extra data must include a section-title key.' );
+		$this->assertEquals( $sectionTitle, $extra['section-title'], 'Detected section title must match' );
+	}
+
+	/**
+	 * @return array All events in db sorted from oldest to newest
+	 */
+	protected function fetchAllEvents() {
+		$res = $this->dbr->select( 'echo_event', array( '*' ), array(), __METHOD__, array( 'ORDER BY' => 'event_id ASC' ) );
+
+		return iterator_to_array( $res );
+	}
+
+	protected function signedMessage( $name, $content = 'Moar cowbell', $depth = 1 ) {
+		return str_repeat(':', $depth)." $content [[User:$name|$name]] ([[User talk:$name|$name]]) 00:17, 7 May 2013 (UTC)\n";
+	}
+
+}