From d0e4aeaecbee9cd940b6616bdf45e4167c0fbe0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Fri, 25 Jun 2021 15:24:46 +0200 Subject: [PATCH] Fix notifications when new comment is under subheading The user interface only allows you to subscribe to level 2 headings. But we would generate events for whatever heading was the closest, If it was e.g. level 3, no one would receive that notification. Now we generate events for the closest level 2 heading, or we don't generate the event at all if there isn't one (if the only headings are of level 3 and below, or level 1, or if the comment is added before the first heading on the page). Bug: T286736 Change-Id: Iae99853070e353ab81c9cc29ef1d53c877adfc66 --- includes/Notifications/EventDispatcher.php | 15 +++++++++++++++ tests/cases/EventDispatcher/section0/rev2.json | 17 +---------------- tests/cases/EventDispatcher/section0/rev3.json | 17 +---------------- .../EventDispatcher/subsection-empty/rev2.json | 17 +---------------- .../EventDispatcher/subsection-empty/rev3.json | 17 +---------------- .../cases/EventDispatcher/subsection/rev3.json | 4 ++-- .../cases/EventDispatcher/subsection/rev4.json | 4 ++-- tests/phpunit/EventDispatcherTest.php | 6 ++++-- 8 files changed, 27 insertions(+), 70 deletions(-) diff --git a/includes/Notifications/EventDispatcher.php b/includes/Notifications/EventDispatcher.php index cb787e3e1..bdabe5878 100644 --- a/includes/Notifications/EventDispatcher.php +++ b/includes/Notifications/EventDispatcher.php @@ -15,6 +15,7 @@ use Error; use IDBAccessObject; use Iterator; use MediaWiki\Extension\DiscussionTools\CommentParser; +use MediaWiki\Extension\DiscussionTools\HeadingItem; use MediaWiki\Extension\DiscussionTools\Hooks\HookUtils; use MediaWiki\Extension\DiscussionTools\SubscriptionItem; use MediaWiki\MediaWikiServices; @@ -143,6 +144,20 @@ class EventDispatcher { foreach ( $newComments as $newComment ) { $heading = $newComment->getHeading(); + // Find a level 2 heading, because the interface doesn't allow subscribing to other headings. + // (T286736) + while ( $heading instanceof HeadingItem && $heading->getHeadingLevel() !== 2 ) { + $heading = $heading->getParent(); + } + if ( !( $heading instanceof HeadingItem ) ) { + continue; + } + // Check if the name corresponds to a section that contain no comments (only sub-sections). + // The interface doesn't allow subscribing to them either, because they can't be distinguished + // from each other. (T285796) + if ( $heading->getName() === 'h-' ) { + continue; + } $events[] = [ 'type' => 'dt-subscribed-new-comment', 'title' => $title, diff --git a/tests/cases/EventDispatcher/section0/rev2.json b/tests/cases/EventDispatcher/section0/rev2.json index 4f31c32ea..fe51488c7 100644 --- a/tests/cases/EventDispatcher/section0/rev2.json +++ b/tests/cases/EventDispatcher/section0/rev2.json @@ -1,16 +1 @@ -[ - { - "type": "dt-subscribed-new-comment", - "title": {}, - "extra": { - "subscribed-comment-name": "h-X-2020-01-01T00:03:00.000Z", - "comment-id": "c-X-2020-01-01T00:03:00.000Z", - "comment-name": "c-X-2020-01-01T00:03:00.000Z", - "content": "E.", - "section-title": "", - "revid": null, - "mentioned-users": [] - }, - "agent": {} - } -] +[] diff --git a/tests/cases/EventDispatcher/section0/rev3.json b/tests/cases/EventDispatcher/section0/rev3.json index b5792eedb..fe51488c7 100644 --- a/tests/cases/EventDispatcher/section0/rev3.json +++ b/tests/cases/EventDispatcher/section0/rev3.json @@ -1,16 +1 @@ -[ - { - "type": "dt-subscribed-new-comment", - "title": {}, - "extra": { - "subscribed-comment-name": "h-X-2020-01-01T00:03:00.000Z", - "comment-id": "c-Y-2020-01-01T00:04:00.000Z-X-2020-01-01T00:03:00.000Z", - "comment-name": "c-Y-2020-01-01T00:04:00.000Z", - "content": "F.", - "section-title": "", - "revid": null, - "mentioned-users": [] - }, - "agent": {} - } -] +[] diff --git a/tests/cases/EventDispatcher/subsection-empty/rev2.json b/tests/cases/EventDispatcher/subsection-empty/rev2.json index 5be96b5da..fe51488c7 100644 --- a/tests/cases/EventDispatcher/subsection-empty/rev2.json +++ b/tests/cases/EventDispatcher/subsection-empty/rev2.json @@ -1,16 +1 @@ -[ - { - "type": "dt-subscribed-new-comment", - "title": {}, - "extra": { - "subscribed-comment-name": "h-Y-2020-01-01T00:01:00.000Z", - "comment-id": "c-Z-2020-01-01T00:02:00.000Z-Y-2020-01-01T00:01:00.000Z", - "comment-name": "c-Z-2020-01-01T00:02:00.000Z", - "content": "E.", - "section-title": "B", - "revid": null, - "mentioned-users": [] - }, - "agent": {} - } -] +[] diff --git a/tests/cases/EventDispatcher/subsection-empty/rev3.json b/tests/cases/EventDispatcher/subsection-empty/rev3.json index 0a0c86303..fe51488c7 100644 --- a/tests/cases/EventDispatcher/subsection-empty/rev3.json +++ b/tests/cases/EventDispatcher/subsection-empty/rev3.json @@ -1,16 +1 @@ -[ - { - "type": "dt-subscribed-new-comment", - "title": {}, - "extra": { - "subscribed-comment-name": "h-Z-2020-01-01T00:03:00.000Z", - "comment-id": "c-Z-2020-01-01T00:03:00.000Z-D", - "comment-name": "c-Z-2020-01-01T00:03:00.000Z", - "content": "F.", - "section-title": "D", - "revid": null, - "mentioned-users": [] - }, - "agent": {} - } -] +[] diff --git a/tests/cases/EventDispatcher/subsection/rev3.json b/tests/cases/EventDispatcher/subsection/rev3.json index f3833691d..4e4f88824 100644 --- a/tests/cases/EventDispatcher/subsection/rev3.json +++ b/tests/cases/EventDispatcher/subsection/rev3.json @@ -3,11 +3,11 @@ "type": "dt-subscribed-new-comment", "title": {}, "extra": { - "subscribed-comment-name": "h-Y-2020-01-01T00:02:00.000Z", + "subscribed-comment-name": "h-X-2020-01-01T00:01:00.000Z", "comment-id": "c-Z-2020-01-01T00:04:00.000Z-Y-2020-01-01T00:02:00.000Z", "comment-name": "c-Z-2020-01-01T00:04:00.000Z", "content": "G.", - "section-title": "C", + "section-title": "A", "revid": null, "mentioned-users": [] }, diff --git a/tests/cases/EventDispatcher/subsection/rev4.json b/tests/cases/EventDispatcher/subsection/rev4.json index 5ae901547..11bd84490 100644 --- a/tests/cases/EventDispatcher/subsection/rev4.json +++ b/tests/cases/EventDispatcher/subsection/rev4.json @@ -3,11 +3,11 @@ "type": "dt-subscribed-new-comment", "title": {}, "extra": { - "subscribed-comment-name": "h-Z-2020-01-01T00:05:00.000Z", + "subscribed-comment-name": "h-X-2020-01-01T00:01:00.000Z", "comment-id": "c-Z-2020-01-01T00:05:00.000Z-E", "comment-name": "c-Z-2020-01-01T00:05:00.000Z", "content": "H.", - "section-title": "E", + "section-title": "A", "revid": null, "mentioned-users": [] }, diff --git a/tests/phpunit/EventDispatcherTest.php b/tests/phpunit/EventDispatcherTest.php index 97836860d..2d8f08698 100644 --- a/tests/phpunit/EventDispatcherTest.php +++ b/tests/phpunit/EventDispatcherTest.php @@ -101,7 +101,8 @@ class EventDispatcherTest extends IntegrationTestCase { 'Z', '../cases/EventDispatcher/multiple/rev2.json', ], - // Adding comments in section 0 (before first heading). + // Adding comments in section 0 (before first heading). These do not generate notifications, + // because the interface doesn't allow subscribing to it. [ 'cases/EventDispatcher/section0/rev1.txt', 'cases/EventDispatcher/section0/rev2.txt', @@ -122,7 +123,8 @@ class EventDispatcherTest extends IntegrationTestCase { '../cases/EventDispatcher/emptysection/rev2.json', ], // Adding comments in sub-sections, where the parent section has no comments (except in - // sub-sections). + // sub-sections). These do not generate notifications because of the fix for T286736, + // but maybe they should? [ 'cases/EventDispatcher/subsection-empty/rev1.txt', 'cases/EventDispatcher/subsection-empty/rev2.txt',