diff --git a/includes/CommentParser.php b/includes/CommentParser.php index d760b3c7d..11476541f 100644 --- a/includes/CommentParser.php +++ b/includes/CommentParser.php @@ -799,6 +799,19 @@ class CommentParser { return $this->commentItems; } + /** + * Get all discussion comments (and headings) within a DOM subtree, grouped by item names. + * + * @return ThreadItem[][] Array where keys are thread item names, values are nonempty arrays of + * thread items + */ + public function getThreadItemsByName(): array { + if ( !$this->threadItems ) { + $this->buildThreads(); + } + return $this->threadItemsByName; + } + /** * Find ThreadItems by their name * diff --git a/includes/Notifications/EventDispatcher.php b/includes/Notifications/EventDispatcher.php index 5fce9eb3e..899e34693 100644 --- a/includes/Notifications/EventDispatcher.php +++ b/includes/Notifications/EventDispatcher.php @@ -13,10 +13,12 @@ use EchoEvent; use Error; use IDBAccessObject; use Iterator; +use MediaWiki\Extension\DiscussionTools\CommentItem; use MediaWiki\Extension\DiscussionTools\CommentParser; use MediaWiki\Extension\DiscussionTools\HeadingItem; use MediaWiki\Extension\DiscussionTools\Hooks\HookUtils; use MediaWiki\Extension\DiscussionTools\SubscriptionItem; +use MediaWiki\Extension\DiscussionTools\ThreadItem; use MediaWiki\MediaWikiServices; use MediaWiki\Page\PageIdentity; use MediaWiki\Revision\RevisionRecord; @@ -103,6 +105,27 @@ class EventDispatcher { self::generateEventsFromParsers( $events, $oldParser, $newParser, $newRevRecord, $title, $user ); } + /** + * For each top-level heading, get a list of comments in the thread grouped by names, then IDs. + * (Compare by name first, as ID could be changed by a parent comment being moved/deleted.) + * + * @param ThreadItem[] $items + * @return CommentItem[][][] + */ + private static function groupCommentsByThreadAndName( array $items ): array { + $comments = []; + $threadName = null; + foreach ( $items as $item ) { + if ( $item instanceof HeadingItem && ( $item->getHeadingLevel() <= 2 || $item->isPlaceholderHeading() ) ) { + $threadName = $item->getName(); + } elseif ( $item instanceof CommentItem ) { + Assert::invariant( $threadName !== null, 'Comments are always preceded by headings' ); + $comments[ $threadName ][ $item->getName() ][ $item->getId() ] = $item; + } + } + return $comments; + } + /** * Helper for generateEventsForRevision(), separated out for easier testing. * @@ -121,16 +144,39 @@ class EventDispatcher { PageIdentity $title, UserIdentity $user ) { - $newComments = []; - foreach ( $newParser->getCommentItems() as $newComment ) { - if ( - $newComment->getAuthor() === $user->getName() && - // Compare comments by name, as ID could be changed by a parent comment - // being moved/deleted. The downside is that multiple replies within the - // same minute will only fire one notification. - count( $oldParser->findCommentsByName( $newComment->getName() ) ) === 0 - ) { - $newComments[] = $newComment; + $newComments = self::groupCommentsByThreadAndName( $newParser->getThreadItems() ); + $oldComments = self::groupCommentsByThreadAndName( $oldParser->getThreadItems() ); + $addedComments = []; + + foreach ( $newComments as $threadName => $threadNewComments ) { + foreach ( $threadNewComments as $commentName => $nameNewComments ) { + // Usually, there will be 0 or 1 $nameNewComments, and 0 $nameOldComments, + // and $addedCount will be 0 or 1. + // + // But when multiple replies are added in one edit, or in multiple edits within the same + // minute, there may be more, and the complex logic below tries to make the best guess + // as to which comments are actually new. See the 'multiple' and 'sametime' test cases. + // + $nameOldComments = $oldComments[ $threadName ][ $commentName ] ?? []; + $addedCount = count( $nameNewComments ) - count( $nameOldComments ); + + if ( $addedCount > 0 ) { + // For any name that occurs more times in new than old, report that many new comments, + // preferring IDs that did not occur in old, then preferring comments lower in the thread. + foreach ( array_reverse( $nameNewComments ) as $commentId => $newComment ) { + if ( $addedCount > 0 && !isset( $nameOldComments[ $commentId ] ) ) { + $addedComments[] = $newComment; + $addedCount--; + } + } + foreach ( array_reverse( $nameNewComments ) as $commentId => $newComment ) { + if ( $addedCount > 0 ) { + $addedComments[] = $newComment; + $addedCount--; + } + } + Assert::postcondition( $addedCount === 0, 'Reported expected number of comments' ); + } } } @@ -142,7 +188,12 @@ class EventDispatcher { } } - foreach ( $newComments as $newComment ) { + foreach ( $addedComments as $newComment ) { + // Ignore comments by other users, e.g. in case of reverts or a discussion being moved. + // TODO: But what about someone signing another's comment? + if ( $newComment->getAuthor() !== $user->getName() ) { + continue; + } $heading = $newComment->getHeading(); // Find a level 2 heading, because the interface doesn't allow subscribing to other headings. // (T286736) diff --git a/tests/cases/EventDispatcher/sametime/rev3-case1.json b/tests/cases/EventDispatcher/sametime/rev3-case1.json index fe51488c7..4cf0c11e1 100644 --- a/tests/cases/EventDispatcher/sametime/rev3-case1.json +++ b/tests/cases/EventDispatcher/sametime/rev3-case1.json @@ -1 +1,16 @@ -[] +[ + { + "type": "dt-subscribed-new-comment", + "title": {}, + "extra": { + "subscribed-comment-name": "h-X-2020-01-01T00:01:00.000Z", + "comment-id": "c-Z-2020-01-01T00:03:00.000Z-X-2020-01-01T00:01:00.000Z", + "comment-name": "c-Z-2020-01-01T00:03:00.000Z", + "content": "F.", + "section-title": "A", + "revid": null, + "mentioned-users": [] + }, + "agent": {} + } +] diff --git a/tests/cases/EventDispatcher/sametime/rev3-case2.json b/tests/cases/EventDispatcher/sametime/rev3-case2.json index fe51488c7..4ab86183d 100644 --- a/tests/cases/EventDispatcher/sametime/rev3-case2.json +++ b/tests/cases/EventDispatcher/sametime/rev3-case2.json @@ -1 +1,16 @@ -[] +[ + { + "type": "dt-subscribed-new-comment", + "title": {}, + "extra": { + "subscribed-comment-name": "h-Y-2020-01-01T00:02:00.000Z", + "comment-id": "c-Z-2020-01-01T00:03:00.000Z-Y-2020-01-01T00:02:00.000Z-1", + "comment-name": "c-Z-2020-01-01T00:03:00.000Z", + "content": "F.", + "section-title": "C", + "revid": null, + "mentioned-users": [] + }, + "agent": {} + } +] diff --git a/tests/cases/EventDispatcher/sametime/rev3-case3.json b/tests/cases/EventDispatcher/sametime/rev3-case3.json index fe51488c7..c81050a55 100644 --- a/tests/cases/EventDispatcher/sametime/rev3-case3.json +++ b/tests/cases/EventDispatcher/sametime/rev3-case3.json @@ -1 +1,16 @@ -[] +[ + { + "type": "dt-subscribed-new-comment", + "title": {}, + "extra": { + "subscribed-comment-name": "h-X-2020-01-01T00:01:00.000Z", + "comment-id": "c-Z-2020-01-01T00:03:00.000Z-X-2020-01-01T00:01:00.000Z", + "comment-name": "c-Z-2020-01-01T00:03:00.000Z", + "content": "E.", + "section-title": "A", + "revid": null, + "mentioned-users": [] + }, + "agent": {} + } +] diff --git a/tests/cases/EventDispatcher/sametime/rev3-case5.json b/tests/cases/EventDispatcher/sametime/rev3-case5.json index fe51488c7..ce6e2fc17 100644 --- a/tests/cases/EventDispatcher/sametime/rev3-case5.json +++ b/tests/cases/EventDispatcher/sametime/rev3-case5.json @@ -1 +1,16 @@ -[] +[ + { + "type": "dt-subscribed-new-comment", + "title": {}, + "extra": { + "subscribed-comment-name": "h-Y-2020-01-01T00:02:00.000Z", + "comment-id": "c-Z-2020-01-01T00:03:00.000Z-Y-2020-01-01T00:02:00.000Z-1", + "comment-name": "c-Z-2020-01-01T00:03:00.000Z", + "content": "E.", + "section-title": "C", + "revid": null, + "mentioned-users": [] + }, + "agent": {} + } +] diff --git a/tests/cases/EventDispatcher/sametime/rev3b-case7.json b/tests/cases/EventDispatcher/sametime/rev3b-case7.json index fe51488c7..6c5c24b00 100644 --- a/tests/cases/EventDispatcher/sametime/rev3b-case7.json +++ b/tests/cases/EventDispatcher/sametime/rev3b-case7.json @@ -1 +1,16 @@ -[] +[ + { + "type": "dt-subscribed-new-comment", + "title": {}, + "extra": { + "subscribed-comment-name": "h-Y-2020-01-01T00:02:00.000Z", + "comment-id": "c-Z-2020-01-01T00:05:00.000Z-X-2020-01-01T00:03:00.000Z", + "comment-name": "c-Z-2020-01-01T00:05:00.000Z", + "content": "H.", + "section-title": "C", + "revid": null, + "mentioned-users": [] + }, + "agent": {} + } +]